WIP: fix(rpc): harden mobile JSI bridge against reader races and stream desync by chrisnojima · Pull Request #29464 · keybase/client

chrisnojima · 2026-07-24T21:39:57Z

The mobile RPC layer had several failure modes that leave the app alive but permanently unable to talk to the service. This fixes them across the C++ JSI bridge, both platform layers, the JS transport, and the Go bindings.

The branch has since been through a multi-reviewer audit and a full fix pass — see Audit below for what that changed, including two regressions the original commits introduced.

Reader lifecycle

Go's ReadArr returns a view of one shared global buffer and is documented as "called serially by the mobile run loops" — and a thread parked inside it cannot be cancelled (Java interrupts and dispatch cancellation are both no-ops there). The old stop-and-restart design (shutdownNow + awaitTermination on Android, readQueue nil-ing on iOS) returned while the previous reader was still live, so a resume or reload could leave two readers racing over that buffer and over the (not thread safe) msgpack::unpacker behind it.

Both platforms now run exactly one reader for the life of the process, forwarding to whichever bridge is currently installed via a mutex-guarded global instead of an instance member. ReadArr also returns a copy rather than a view, so a stray second reader can no longer corrupt an in-flight delivery — serial access is still required for conn.Read ordering and the downstream unpacker, but the memory-corruption hazard is gone.

teardown() (destroys jsi handles, JS-thread-only) is split from markTornDown() (atomic flag, any thread). Module invalidation uses the latter.

Teardown

Both platforms tore down the wrong bridge under a reload.

iOS: invalidate cleared whichever bridge was installed, not the one that module installed. RCTInstance::invalidate hops to the old JS thread asynchronously and RCTTurboModuleManager waits at most 10s before proceeding, so a stale invalidate could land after the next module installed — killing the live bridge with no desync, no reset, and no recovery. Now compare-and-clear, with the Go reset gated on the same check. kbSharedInstance is cleared too: RN never nulls _eventEmitterCallback, so canEmit otherwise stays true for a dead module.
Android: bridge teardown ran from onHostDestroy, which fires when the last Activity dies while the ReactInstance survives (the ReactHost is application-scoped). Back button to home, then reopen, left the bridge permanently null. It now runs from invalidate(), with the same identity guard as iOS.

Stream desync and connection resets

The framing state machine alternated needSize/needContent with no validation. One malformed frame flipped the parity permanently and every later message was silently swallowed — with no way back, since mobile's transport reports isConnected() unconditionally and Engine.reset() was a no-op there.

Framing is now self-checking: the size prefix must be a msgpack uint within the frame limit, and the content object must consume exactly the declared byte count. A mismatch resets the unpacker and drives onFatal, which resets the Go connection and emits kb-engine-reset.

The reset story is wired for every trigger, not just desync:

A read error (the common case — ReadArr resets Go itself and returns) now notifies JS, so outstanding RPCs fail instead of hanging behind stale spinners.
The reset path reports the disconnect before the reconnect, so sessions are cancelled and the UI shows a reconnect state.
Resets are epoch-checked: every successful dial bumps a generation counter, and failure-driven callers reset only the connection they actually saw fail. A stale complaint can no longer close a healthy connection that another path just re-dialed and wrote to. Genuine teardown callers (invalidate, destroy, engineReset) stay unconditional.
Emits are backed off, so a connection that cannot be re-dialed doesn't drive _onDisconnect/_onConnected — which are not idempotent — at 10Hz.

Hanging invocations

Nothing failed the outstanding invocation map on mobile, so an engine reset or a dropped native write left every in-flight RPC waiting forever. kb-engine-reset now fails outstanding invocations before signalling reconnect, rpcOnGo reports write failures back to JS, and invokeNow fails the caller if the message never left.

Response settlement is idempotent: makeResponse settles exactly once, so an auto-acked call whose handler then throws can't double-answer a seqid. A throwing incoming-invoke handler now errors its response instead of leaving the service waiting forever.

On desktop, packetizeData dispatched inside the framing try, so any app-code throw unwound into the catch that resets the packetizer — parsing then resumed mid-stream and never recovered on the renderer transport. Dispatch is now isolated from framing.

Conversion

Both msgpack↔JSI walks are iterative, so nesting depth costs heap instead of native stack.
Typed arrays are detected with ArrayBuffer.isView, replacing a first-key-is-a-digit heuristic that could route a plain object into an unvalidated out-of-bounds read. Every range is bounds-checked.
Symbols and BigInts pack as nil rather than packing nothing, which had corrupted the enclosing map (N keys, N-1 values).
Conversion is per-message: one undecodable message is dropped alone rather than taking its whole batch — and the batch shape now depends on how many messages actually survived, so a batch of one is delivered unwrapped rather than as an array JS would reject.
The receive buffer releases its peak at a safe resync point instead of pinning up to 64MB for the session.

Audit

Five independent reviewers audited the original commits and found 20+ defects; the fix pass ran task-by-task with a review gate on each and a whole-branch review at the end. Notable outcomes:

Two regressions the original commits introduced were caught and fixed: the Android teardown-on-Activity-death above, and an iOS emit that could fire into a dying runtime.
One planned fix was withdrawn after review showed it was both unnecessary and harmful — applied in 0ed435d, reverted in 2546936. It was a latch meant to stop one desync from firing a reset per in-flight chunk. Two problems: the storm was already impossible, because onFatal_ runs synchronously on the single reader thread, so KeybaseReset completes before the next ReadArr and the stale bytes are discarded with the closed connection. And every way of clearing the latch was broken — clearing it inside the fatal handler is a no-op on the same synchronous call chain, while clearing it only on a read error wedges permanently, since the post-reset reconnect succeeds and never produces the error that would clear it.
Several comments asserted invariants the code did not have and are corrected — most notably the claim that ReadArr polls and returns nothing when idle. It blocks; the sleep those comments justified was dead code, and the empty-read branch is a degenerate case reachable only if Init never ran.

Three commit messages overstate their fix and are corrected here:

Commit	Correction
`7682e02`	The duplicate-fallback-notification symptom was overstated — `isReactNativeRunning()`'s only caller logs the value and never branches on it. The wedge itself was real.
`def5fb4`	Described as the mirror of the invoke-direction hang fix. It is observability only: a failed response write still leaves the service hung on that seqid, it is now merely logged.
`fa04cc0`	Used `static` linkage to resolve a duplicate `quarantineFile` symbol; superseded by `04f4d99`, which renames both shims instead.

Testing

This branch adds the first automated coverage for this layer. go/bind had no test file; the C++ framing logic had no test target at all.

Suite	Before	Now
`go test ./bind/... -race`	none	15
C++ framing (`rnmodules/react-native-kb/scripts/test-framing.sh`)	none	14
`yarn jest engine/`	22	42 (5 suites)

Getting the framing logic under test required extracting it into a standalone FrameParser with no JSI dependency — that code had already shipped two serious bugs during this effort (arithmetic that would have rejected every frame, and a buffer-shrink trigger that could never fire), both caught only by review. The extraction was verified line-by-line for behavior equivalence, and the test binary links the shipped source rather than a copy.

Tests were falsified where it matters: the production code was temporarily broken to confirm each key test actually catches the regression it claims to.

Also green: yarn lint:all (0 react-compiler bailouts, tsc clean on both configs), Android :externalNativeBuildDebug + :compileDebugKotlin, iOS xcodebuild -scheme react-native-kb, go build/go vet ./bind/....

iOS device pass

Validated on the simulator with temporary instrumentation (since removed). Results:

Flow	Result
Cold start	PASS — exactly one reader thread, one bridge install, first dial at epoch 1
Dev reload ×5	PASS — reader stayed at exactly 1; every `invalidate` tore down its own bridge pointer, never a successor; epochs advanced 1→6; zero desyncs
Background / foreground	PASS — no reader restart, no bridge teardown, no reconnect; same connection throughout
Chat traffic + DB nuke + re-sync	PASS — zero `bad rpc frame header`, zero `rpc frame length mismatch`, zero `rpc stream desync`, zero write failures across ~40k chat log lines

The reload case also exercises the read-error leg on every iteration: invalidate resets the Go connection, and the parked ReadArr returns EOF. It correctly declines to emit an engine-reset into a module that is already being torn down.

The device pass also removed a change. An earlier commit made the mobile account switch fail in-flight invocations, on the theory that a pre-switch callback could otherwise fire against post-switch state. Instrumenting the switch disproved it: across four switches only two invocations were ever outstanding at NotifySession.loggedOut, and both were the switch's own machinery — login.login and login.getConfiguredAccounts. No stale data RPC crossed. And because login.login is the call performing the switch, failing outstanding invocations there would have EOF'd the login on every switch. Reverted in 20af0db.

That change was also unreachable in practice: the account-switch call sites live in the desktop half of onEngineIncoming's isMobile split, so Engine.reset() never ran on that path at all. Static review checked that the file was shared; only running it exposed the branch.

Not covered

Android has not had a device pass. The highest-value case is back button to home → reopen → send/receive: that wedge broke, was fixed, was silently re-broken by a later commit in this branch, and was fixed again.
A read error with a live module (the emit=1 → JS kb-engine-reset → fail-outstanding chain) can't be produced on the simulator, since the service is in-process and can't be killed independently. That path is covered by the Go tests only.
Possible pre-existing desktop issue, unverified: desktop's loggedOut case calls getEngine().reset(), which by the same mechanism may EOF its own in-flight login.login on every account switch. Worth a separate look; untouched here.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

zoom-ua · 2026-07-28T15:19:56Z

+                            size_t size, const Value &value) {
+  // RPC maps reuse a small set of string keys; caching the PropNameIDs
+  // avoids re-interning the same symbols on every message. The cache is
+  // never cleared once full — maps keyed by dynamic IDs would otherwise


how did you decide this was the best approach? was the performance of this change measured?

Measured now — and the answer splits in two, because most of what that hunk shows isn't actually new.

The PropNameID cache predates this PR. It's master's mapKeyPropName, comment and 4096 cap included; it reads as added because the whole conversion block moved in the diff. What genuinely changed is the overflow policy. Master calls cachedPropNames_.clear() once the map is full while handing callers a const PropNameID& into that map, so the clear invalidates a reference the caller may still be holding. This version stops caching past the cap and builds a throwaway instead, and setObjectKey consumes the id on the spot rather than returning it. That was the reason for the change — the dangling reference, not throughput.

Cache on vs off, same build otherwise, two runs each:

decode encode

cache off 7.42 / 7.67 ms 8.80 / 8.99 ms

cache on 7.00 / 7.07 ms 8.90 / 8.87 ms

~7% of decode. It's worth having because the traffic repeats keys hard — 20,286 map keys across 820 distinct strings, implicitRole and role alone at 3,658 hits each — but it isn't load-bearing.

I also checked whether an LRU would do better, since 4096 looks like a limit worth raising: it wouldn't. 820 distinct keys means the cap never binds and there are zero evictions, so the cache already sits at its ceiling hit rate. The cap only engages on maps keyed by dynamic ids (uids, convIDs), and those keys never recur — LRU would evict live keys to store dead ones and come out behind the current policy.

Branch vs master end-to-end, best of 5 over 9.5MB of real traffic:

decode msgpack→JSI encode JSI→msgpack

master 6.6 ms 8.6 ms

this branch 6.8 ms 8.8 ms

Parity; the gap is inside run-to-run spread. The iterative walk costs nothing measurable — it's there for stack safety on deep payloads, not speed. The first run of this did show encode 24% slower, all of it packNumber's floor()/isfinite() (neither inlines, and every number on the wire goes through it). Rewritten as a range check plus an integer round-trip in 4b3d212, verified identical over the boundary values and 6M random doubles.

Methodology, since "measured" is only worth as much as the corpus: scripts/make-bench-corpus.mjs pulls real payloads out of a mobile session's Metro log (printRPC records everything the app received) and re-encodes them with the same @msgpack/msgpack and framing the transport uses — 2,424 frames, 9.5MB, 4KB mean, mostly chat inbox conversations. cpp/tests/jsi-convert-bench.cpp replays it on real Hermes from the macOS slice the iOS pod already ships, driving both directions through the public API in reads capped at Go's ReadArr size. scripts/bench-jsi-convert.sh builds it against both branches and runs them.

Worth flagging: building this found a real bug, fixed in e21af9c. FrameParser::reset() was move-assigning msgpack::unpacker, which doesn't survive it — the parser base holds the referenced-buffer hook as a reference member that the move constructor copies instead of rebinding, leaving it pointed at the dead temporary. It only detonates when the buffer expands mid-frame, i.e. the next sizeable message after any reset, which is exactly the error-recovery path this PR adds. Every existing reset test passed because they fed small complete frames that never grow the buffer; there's a regression test now that segfaults without the fix.

…sync The mobile RPC layer had several failure modes that leave the app alive but permanently unable to talk to the service. Reader lifecycle. Go's ReadArr returns a view of one shared global buffer and is documented as "called serially by the mobile run loops", but a thread parked inside it cannot be cancelled — Java interrupts and dispatch cancellation are both no-ops there. The old stop-and-restart design (shutdownNow + awaitTermination on Android, readQueue nil-ing on iOS) returned while the previous reader was still live, so a resume or reload could leave two readers racing over that buffer and over the (not thread safe) msgpack::unpacker behind it. Both platforms now run exactly one reader for the life of the process, forwarding to whichever bridge is currently installed via a mutex-guarded global rather than an instance member — which also removes the unsynchronized shared_ptr access between the reader thread and installJSIBindings/invalidate. Stream desync. The incoming framing state machine alternated needSize and needContent with no validation, and left the unpacker untouched on error. One malformed frame flipped the parity permanently and every later message was silently swallowed as a "size", with no way back: mobile's transport reports isConnected() unconditionally and Engine.reset() is a no-op there. The size prefix is now checked to be a msgpack uint within the frame limit, and a failure resets the unpacker and drives a new onFatal path that resets the Go connection and emits kb-engine-reset. Hanging invocations. Nothing failed the outstanding invocation map on mobile, so an engine reset or a dropped native write left every in-flight RPC waiting forever. kb-engine-reset now fails outstanding invocations before signalling reconnect, rpcOnGo reports write failures back to JS, and invokeNow fails the caller if the message never left. Conversion. Both msgpack<->JSI walks are now iterative, so nesting depth costs heap instead of native stack. Typed arrays are detected with ArrayBuffer.isView behind a cheap byteLength probe, replacing a first-key-is-a-digit heuristic that could route a plain object into an unvalidated out-of-bounds read of the backing buffer; every range is now bounds-checked. Symbols and BigInts pack as nil instead of packing nothing, which had been corrupting the enclosing map. The PropNameID cache no longer clears itself while entries are referenced, the outgoing scratch buffer is released after an oversized frame, and rpcOnJs is re-read per batch so a recreated engine client is never fed to a dead handle. Also: per-message try/catch in the batch dispatch loop so one bad message cannot drop the rest of the batch, and idle-read sleeps on both platforms (ReadArr returns nothing when the connection is idle, which was spinning a core).

The process-wide read loop forwards to KbModule.instance, so a torn-down module kept receiving deliveries and pinned its ReactContext. Clear it in destroy(), guarded so a reload that installs a newer module first wins. Add transport tests for native write failures: invoke fails the caller, leaves no outstanding seqid, doesn't steal a later invoke's response, and send() reports false.

The reader block never returns, so the queue's pool never drains and autoreleased objects accumulate for the life of the process.

Covers: single/coalesced/split frames, a split at every byte boundary, consumed==declared across sizes including an unpacker buffer growth, bad/oversized/mismatched/zero-length headers, a legal maximal frame delivered in chunks, error recovery after reset, the peakFrameSize buffer-shrink trigger surviving a subsequent small frame, and nested/ empty container round-trips. No gtest/catch2 dependency: a small assert/report harness (test-harness.h) keeps this buildable with the same plain clang++ already used for syntax-checking react-native-kb.cpp. Run via plans/scripts/test-native-kb-framing.sh.

A post-reset invocation must get a seqid that cannot alias a pre-reset one, and a stale response for a pre-reset seqid arriving after the reset must be ignored rather than re-firing an already-failed callback.

…ailed-write error() then result() on the same response must settle on the first call (error), matching the already-covered result()-then-error() case. A response whose write fails is marked settled before the write is attempted, so retrying result() after fixing the write error must trip the double-settle guard rather than silently opening a retry path.

…undary A malformed frame must reset the packetizer and let a subsequent well-formed frame dispatch normally -- genuine corruption still recovers. Also sweep every possible split point of one frame across two packetizeData calls (rather than only the byte-at-a-time case) to pin down the split-boundary framing logic more broadly.

…Go failure Exercises index.platform's isMobile branch directly: a throwing disconnectCallback must not skip connectCallback on kb-engine-reset (the stranded-banner fix), and NativeTransportMobile must fail an invocation rather than hang when rpcOnGo reports a failed native write.

chat/attachments and kbfs/simplefs each defined a C function named quarantineFile with an identical body. Nothing linked both into one darwin binary before, so the collision was latent; go/bind imports both, and any executable pulling it in (a go test binary, for one) failed to link with a duplicate symbol error. Rename each to match its package rather than relying on internal linkage, so the symbols stay visible under their own names in a stack trace or nm dump.

Platform readers (Kb.mm, KbModule.kt) call ResetIfCurrent(epoch) and then unconditionally clear their local parser state. When the epoch is stale (some other caller already redialed), that clearing still happens and drops bytes already in flight on the connection nothing here touched, forcing a second, avoidable fatal/reset cycle. ResetIfCurrentDidReset reports the acted/no-op distinction so callers can gate on it. Adds Go coverage for the two real production callers of the mechanism (ReadArr's error path, WriteArr's short-write path), for the new reporting function, and for the epoch-capture-before-raceable-read property the whole design rests on (falsified by moving the lastReadEpoch write after conn.Read, confirming the new test catches the regression).

resetRecv()/nativeResetRecv() ran unconditionally after ResetIfCurrent(epoch), even when it was a stale no-op. That drops a partial frame already buffered on a connection some concurrent redial already recovered, forcing a needless second fatal/reset cycle. Both platforms now call ResetIfCurrentDidReset and only clear the parser when it reports the reset actually happened.

RecvState::epoch was written on every onDataFromGo call but never read anywhere; the fatal paths use the epoch captured in onDataFromGo's own parameter/lambda instead, since resetRecvLocked() used to discard the whole RecvState (and epoch with it) before those paths could run. With epoch gone, RecvState owns only the parser, so resetRecvLocked() can reset it in place via FrameParser::reset() instead of reconstructing RecvState from scratch, avoiding a reallocation on every safe-shrink resync.

…ments pack_uint64 picks the minimal msgpack encoding, so small test frames got a 1-byte fixint header while production always writes a fixed 5-byte 0xce+uint32 header. That gap meant the split-at-every-byte-boundary sweep never split inside a real header, and the max-frame test put the whole header in the first chunk — exactly where consumedAtHeader_ is most fragile. Adds packHeaderUint32 matching production's encoding and reruns the sweep with it, plus a header-boundary sweep for a kMaxFrameSize frame. Falsified against a broken consumedAtHeader_ computation (using the current feed()'s size instead of cumulative totalFed_): only the new production-header sweeps and the existing multi-feed tests caught it, confirming the new coverage closes the gap. Also corrects two comments that overclaimed what they tested: frame-parser-test.cpp's "proves kMaxFrameSlack" claim actually pins a header boundary condition (next() drains nonparsed_size() to 0 before the slack check runs), and frame-parser.h's kMaxFrameSlack rationale described a legal-frame scenario that can't occur (declared sizes are already bounded by kMaxFrameSize at header time). kMaxFrameSlack is kept since dropping it would be a real, if minor, garbage-detection behavior change; only the rationale is corrected to describe its actual purpose.

- Drop fragile log-count/substring assertions in rpc-transport.test.ts that redden on any reworded/added log line with zero behavior change; the load-bearing assertions (transport.sent, error counts) already cover the real behavior. - Simplify the reset-cycle seqid test: drop a redundant restatement of the adjacent ordering test and a tautological callback-count check that asserted a dispatch to a map key the callback was never registered under, keeping the one property (a fresh post-reset call round-trips end to end) nothing else in the file covers. - Assert that transport.reset() actually fires on kb-engine-reset in index.platform.mobile.test.ts, instead of letting it execute unasserted. - Pin the exact error message so the malformed-input test can tell which of two throw sites fired, instead of a bare toBeTruthy(). - Fix teardownMobileMocks leaking global.rpcOnJs across test files. - Drop the local react-native jest.requireActual workaround, which doesn't bypass moduleNameMapper and so re-spread the shared stub for no effect; add the LogBox export it needed to the shared mock instead. - Remove a throwaway TestTransport built only to derive a seqid that's deterministically 1 and already hardcoded elsewhere in the file.

These are build and test tooling, not planning documents, so they belong next to the module they operate on. plans/ holds scratch working notes that are not part of the codebase; ignore dated plan files so they stop being committed.

Added on the theory that a mobile account switch could let a pre-switch RPC callback fire against post-switch state. Runtime validation on the simulator disproved it: across four switches only two invocations were ever in flight at NotifySession.loggedOut, and both were the switch's own machinery -- login.login and login.getConfiguredAccounts. No stale data RPC crossed the boundary. Worse, login.login is the call that performs the switch; the service emits loggedOut while still handling it. Failing outstanding invocations there would EOF the login itself on every switch. The line was also unreachable on mobile in practice: the account-switch call sites live in the desktop half of onEngineIncoming's isMobile split, so Engine.reset() is never invoked on that path. Keeps the tests added alongside it -- they exercise failAllOutstanding and seqid behavior on the transport directly and stand on their own.

…n wedge Static check that nativeInvalidate() is only called from invalidate() (real TurboModule teardown) and never from destroy()/onHostDestroy (Activity death with a surviving ReactInstance) — the bug fixed in 15647bb. Parses actual function bodies via brace matching so it survives reformatting rather than a naive substring scan.

New Android-only flow: destroy the Activity via back (not terminateApp) so the process/ReactInstance survive, reopen via activateApp, and prove inbound RPC still works with a full chat send/receive round trip. Verifies process continuity via `mobile: shell pidof` before/after and refuses to pass if the PID changed, since a killed process would reinstall the bridge and mask the bug. Needs relaxedSecurity on the Android appium service for the shell command.

…t test relaxedSecurity applies to the whole appium server, not just the one command that needs it, so enabling it by default widens what a locally spawned appium will execute for every Android run. Gate it behind KB_E2E_RELAXED_SECURITY instead. The activity-restart test needs `mobile: shell` (pidof) to distinguish a surviving process from a fresh one -- a fresh process reinstalls the bridge and would pass while proving nothing. Without the flag it now skips with a message saying so, rather than reporting a result it cannot substantiate.

reset() rebuilt the unpacker with `unpacker_ = msgpack::unpacker()`, which is not a safe operation on this type. msgpack::unpacker has no move assignment of its own, so it inherits its parser base's, and that base holds the referenced- buffer hook as a *reference* member bound to the finalizer living inside the unpacker object. The move constructor copies that reference rather than rebinding it, so a move-assigned unpacker keeps pointing at the source -- here a temporary that dies at the end of the statement. Nothing goes wrong until the buffer has to grow while it is still referenced, which is what happens when a frame arrives across several reads and a string has already been parsed out of it. expand_buffer then calls through the dangling reference into a dead stack frame. That is the ordinary reader pattern for any frame bigger than one read from Go, so every reset -- connection replaced, framing error recovery, buffer shrink -- armed a crash for the next sizeable message. Hold the unpacker by unique_ptr and replace it wholesale instead. The regression test reproduces the original crash: it resets from its own stack frame, paints that frame over, then feeds a two-field frame in chunks so the buffer expands mid-parse. Without the fix it segfaults; the existing reset tests all passed because they only fed small complete frames, which never expand the buffer.

packNumber ran `d == std::floor(d) && std::isfinite(d)` on every number headed for the wire. Neither call inlines, and the benchmark against real captured traffic put the pair at roughly 20% of total pack time -- the single largest difference between this branch and master on the send path. Range-check first so the cast is always defined, then let the round-trip through the integer prove d was integral. Same output for every input: verified against the old formulation over the boundary values (zero, negative zero, 2^53, +/-2^63, 2^64, the largest double below 2^64, the infinities, NaN) and six million random doubles, with zero mismatches. NaN and the infinities fail both range checks and fall through to pack(d) exactly as they did before. Encode is back to parity with master on the benchmark corpus.

There was no way to answer what the msgpack <-> JSI conversion actually costs, so review questions about it could only be answered by argument. This measures it, on real payloads rather than invented ones. make-bench-corpus.mjs reads a Metro dev log from a real mobile session, where printRPC has already recorded every payload the app received, and re-encodes the ones Metro logged in full using the same @msgpack/msgpack and the same framing the transport writes. A recent session yields 2,424 frames and 9.5MB at a 4KB mean, dominated by chat inbox conversations -- the real mix, not a guess. The script reports what it had to drop (Metro elides objects past its depth limit) so the coverage is visible rather than implied. jsi-convert-bench.cpp replays that corpus on a real Hermes runtime, taken from the macOS slice the iOS pod already ships, so nothing has to be built from source. Both directions go through the public API -- frames into onDataFromGo in reads capped at Go's ReadArr buffer size, then whatever rpcOnJs received back out through rpcOnGo -- rather than through internals copied out of the implementation, so the numbers describe the path the app takes. bench-jsi-convert.sh builds it twice, once against this branch and once against master's bridge, and runs both. Current numbers: decode and encode are both at parity with master, and the PropNameID cache is worth about 7% of decode (the corpus has 20,286 map keys across 820 distinct strings).

Android installed g_adapter in getBindingsInstaller but g_bridge only later, from the bindings callback. An invalidate landing in that window cleared both slots and the callback then resurrected a bridge pointing at a runtime that was going away, with no adapter. The reverse ordering was just as bad: a new adapter could be transiently paired with the previous module's bridge, so an invalidate from the new module tore down the wrong one. Installing an adapter now takes over under a single lock -- any existing bridge is moved out and marked torn down, since a new module installing means the previous one is definitively dead. The bridge publish is then identity-gated on the adapter still being current; a bridge installed into an already-invalidated module's runtime is marked torn down instead of published. markTornDown stays outside g_mutex. iOS had the same shape: myBridge_ was assigned under kbBridgeMutex, the lock released, and only then did kbSetBridge publish. An invalidate in between saw a bridge that was not yet current, skipped teardown and the Go reset, and installation went on to publish an already-invalidated bridge that nothing would ever clean up. kbSetBridge becomes kbSetBridgeLocked and both writes now happen in one critical section, with the displaced bridge torn down after unlocking. Android's invalidate also left the Go loopback connection alive, so a replacement JS transport started with fresh seqids and an empty parser while the live connection could still hold outstanding replies or a partial frame -- mis-settling a new invocation with an old response, or desyncing immediately. nativeInvalidate now reports whether it actually cleared, and Kotlin resets Go only when it did, mirroring iOS's identity-gated KeybaseReset. Finally, onRpcStreamFatal skipped nativeResetRecv() when resetIfCurrentDidReset threw, leaving the parser holding bytes from a stream already known to be desynced. The parser is now reset when the Go reset ran or when the call threw; only the clean stale-epoch path still skips it, since that is the one case where the buffered partial frame belongs to a healthy connection.

RPCTransport.close() cleared _pending without settling it, failing only _invocations. An invoke queued while disconnected and then closed was dropped silently, so its caller hung forever -- the same failure shape as the queueMax overflow branch, which does error its callback. It is reachable: LocalTransport (mobile, desktop renderer) overrides close() to a no-op, but desktop non-renderer NativeTransport starts disconnected and calls super.close(), and resetClient() closes it without checking whether the unix socket ever connected. So an engine reset in the main process while the service socket is down drops every queued invoke. failPending detaches the queue before settling, which is what makes it once-only: a later flushPending sees an empty array, so nothing is re-sent or double-settled. Queued raw sends have no callback and are simply dropped. This cannot reach the mobile account switch, where failing outstanding RPCs EOFs login.login: Engine.reset() early-returns on mobile, NativeTransportMobile inherits LocalTransport's empty close(), and failPending only ever touches _pending, never _invocations. Tests cover the packetizer's frame-too-large and queue-overflow branches, flushPending re-entrancy against a failing write, dispatchRpcBatch's outer guard (previously unreachable, since the existing test wrapped dispatchOne in a swallowing helper), ProxyNativeTransport.reset, resetClient's non-renderer branch against a mocked socket, and the close() regression above. The seqid tests now assert post-reset seqids are absent from the pre-reset set rather than merely larger, which is what their names claimed. Also corrects NativeTransportMobile.reset()'s comment: it is reached from the kb-engine-reset meta event, not from an account switch calling Engine.reset(), which early-returns on mobile.

The engine-reset emit backoff and the read-error log counters were untested arithmetic buried in a private inner class on Android and in file-scope statics on iOS. Both matter: too aggressive a backoff and JS re-runs the full session-cancel and bootstrap sweep at ~10Hz, too conservative -- or advancing on an undeliverable emit -- and one dropped notification costs a multi-second window with the UI stuck on the disconnect banner. The split EOF/non-EOF counters exist so a sustained EOF flood cannot let a shared modulus swallow the first non-EOF exception, which is the one log line carrying a stack trace. Android gains ReadLoopThrottles.kt (LogThrottle, ReadErrorLogThrottle, EngineResetEmitThrottle with an injectable clock) and 13 JUnit tests; iOS gains the header-only kb::EngineResetEmitBackoff and 7 equivalent C++ cases needing only a compiler. ReadErrorLogThrottle deliberately owns both counters so "merge them back into one" is a mutation inside the tested unit rather than at an untestable call site. Behavior-preserving apart from one checked difference: iOS now evaluates `deliverable` on every failed read instead of only after the window elapses. canEmit is a pure null check, and Android already evaluated it eagerly, so the two platforms now match. Every case was falsified against a mutated production line -- dropping the undeliverable guard, removing the ceiling clamp, never doubling, merging the counters, starting already backed off, and no-oping reset. The call sites in Kb.mm and KbModule.kt landed with the preceding lifecycle commit, so that commit does not build on its own; the tip does.

…branches packNumber was the highest-risk untested change on the branch: a hand-rolled range check replaced an obvious floor()/isfinite() formulation purely for speed, verified once by a throwaway script and never committed. It is now pinned byte-for-byte against that original formulation, included verbatim as an oracle, over a boundary table (zero, negative zero, 2^53 and neighbours, both sides of +/-2^63, nextafter(2^64,0), 2^64, the infinities, NaN, denormals) plus 1,000,000 seeded doubles. It is identical everywhere tested. An off-by-one between < and <= on 2^64 would send a float64 where Go expects a uint -- silent and data-dependent. msgpack <-> JSI conversion had no correctness tests at all. jsi-convert-test.cpp drives a real Hermes runtime through the public API and covers BIN to Uint8Array, Symbol and BigInt to nil (they previously packed nothing, corrupting the enclosing map), ArrayBuffer.isView detection replacing the first-key-is-a- digit heuristic, typed-array byteOffset/byteLength bounds, non-scalar map keys, kMaxDepth, EXT, empty containers, and UTF-8 with embedded NULs. It also covers the three batch-delivery fixes: a single message must arrive unwrapped, an empty batch must escalate to onFatal, and one undecodable message must drop alone. packAndSend's actual output is fed back through FrameParser at every byte split, so producer and consumer can no longer drift silently. Three parser branches were unreachable from the old suite: the kMaxFrameSlack guard (the existing test rejected at the header check instead), the false cases of atSafeShrinkPoint (a `return true;` stub passed the whole suite), and recovery when the tail of a desynced frame is still arriving. Two tests caught a bare runtime_error without checking the message and could not tell "rejected for the right reason" from "threw from msgpack internals". go/bind gains a private-copy test, a WriteArr epoch-capture race mirroring the ReadArr one, and real coverage of the jsReadyCh gate via a re-exec'd child (the sync.Once made it untestable in-process, which the file called a latent trap). TestConcurrentRedialsAndResets took connMutex itself and called ensureConnection directly, proving the test's own lock discipline rather than the callers'; it now drives ReadArr/WriteArr. The double-reset test passed against a no-op resetLocked and now pins the close count. The concurrent-ReadArr test could not be written: two callers race on the package-level buffer global, deterministically under -race. That is the documented single-permanent-reader invariant, not a bug, so the reasoning is recorded in the test file rather than asserting a property the code lacks. Every new concurrency and guard test was falsified against a mutated production line before being kept.

…eens go/bind's 617-line suite never ran. The Jenkinsfile excluded the package in three places -- the nix and windows test-dir lists and getPackagesToTest -- so every epoch and reset regression the branch's design rests on could rot unnoticed. The exclusion predates the duplicate quarantineFile C symbols being given internal linkage, which was done precisely so the package could link a test binary. It now runs as a targeted `go test -race` step on Linux; the generic sweep is unchanged, and Windows is untouched. The C++ framing and backoff suites were invocable only by hand. Both need just a compiler, so they run ahead of the JS suite -- they take seconds, and failFast would otherwise hide them whenever JS is red. test-framing.sh had to learn to fetch the msgpack headers itself, since yarn-helper's getMsgPack() is gated on darwin and CI is Linux; that fetch now lives in one sourced helper rather than being copied per script. Two tests reported success without testing anything. The Android activity-restart e2e test returned early when KB_E2E_RELAXED_SECURITY was unset, and returning from an async Mocha test marks it passed -- so default runs showed green coverage for a regression scenario that never ran. It now skips, which reports honestly. jsi-convert-bench counted conversion errors, printed them, and returned 0 regardless, so a build where every message failed to convert exited successfully; it now exits nonzero, making it a smoke test for free.

Review fixes from a whole-branch pass: - onFatal identity gate (Kb.mm, cpp-adapter.cpp): a batch queued by a dying runtime can hit its conversion-failure fatal after a reload has published the next bridge. The epoch check can't catch that (nothing re-dialed), so the old runtime would reset the connection the new one is using and clear its parser mid-frame. Only act when the faulting bridge/adapter is still the installed one. iOS captures the bridge weakly (strong capture would cycle through onFatal_). - invokeNow write-throw now fails the caller with the transport error shape (code/desc) instead of the raw exception, so convertToError yields an RPCError like every other transport failure path. - Document the accepted displaced-bridge byte-drop window in getBindingsInstaller and correct packNumber's @msgpack/msgpack comparison (integers above 2^53 differ, benignly, for typed decode).

The Linux CI image's default g++ rejects -std=c++20 outright ("did you mean '-std=c++2a'?") while still implementing enough of C++20 under the older spelling. Newer toolchains only accept -std=c++20, so neither flag works everywhere hardcoded. cxx-select.sh picks a compiler (preferring versioned g++ binaries over a stale default) and probes for the newest C++20 flag it accepts.

golangci-lint's gosec pass flags exec.Command in TestReadArr_BlocksUntilJSReady as a tainted-subprocess risk. The argument is os.Args[0] — this test binary re-executing itself with constant flags — so there is no external input to taint.

These tests created their temp config dirs inside the package directory and relied on a deferred RemoveAll to clean up. That defer does not run when the test binary is killed or times out, so the dir survives. Jenkins reuses workspaces, so a leftover dir gets picked up by the `git add -A` before the protocol regen step and fails the ./go/ ./protocol/ staleness check with a misleading "please run make in protocol" message. t.TempDir() puts the dir outside the repo and the testing package removes it. Also gitignore the old pattern to neutralize dirs already stranded in existing CI workspaces.

chrisnojima requested review from Copilot and zoom-ua July 24, 2026 21:40

Copilot started reviewing on behalf of chrisnojima July 24, 2026 21:40 View session