Review fixes for the zenoh-flat transition

[milyin]

Fixes found reviewing #669 end to end (build + jvmTest against the sibling
zenoh-flat-jni / zenoh-flat commits CI pins).

ZBytes publication race

ZBytes.bytes is double-checked locking over a non-volatile field:

private var eager: ByteArray?
get() = eager ?: synchronized(this) { eager ?: run { … eager = b; b } }

The fast path reads eager outside the monitor, so there is no
happens-before edge to the write inside it. A received ZBytes is exactly
the object that crosses threads — the native callback thread builds the
Sample, a consumer thread pulls it off a Channel and touches the payload
— so a reader can observe the array reference while its copied contents are
still not visible. eager is now @Volatile, which restores the edge.
It moves out of the constructor parameter list because Kotlin only accepts
the annotation on a body property.

ReplyKeyExpr declaration order

The constants were swapped to MATCHING_QUERY(1), ANY(0), with a comment
saying the order "deliberately differs" from the wire values. But the wire
values are carried in value and fromInt looks them up by value, so the
declaration order buys nothing — while reordering an enum silently changes
ordinal() and the order of values()/entries for every consumer, which
is a real source- and behaviour-level break for anyone who persisted or
switched on them. Restored to ANY(0), MATCHING_QUERY(1), with a test in
SelectorTest that pins it.

Build script indentation

Two lines in build.gradle.kts lost their indentation when the
rust-android-gradle entries next to them were removed.

Stale test comment

AdvancedPublisherTest's KDoc says AdvancedPubSubTest "remains
@Ignore'd until Round 2". It is not ignored and it passes. Replaced with
what the file actually contributes: the same advanced publisher exercised
against a plain subscriber, which AdvancedPubSubTest does not cover.

---

Verification: ./gradlew jvmTest -PuseLocalFlatJni=true with
zenoh-flat-jni@6f81eb8 and zenoh-flat@81feb94 — 122 tests, all green
(121 before, plus the new one).

[milyin]

Reviewed the complete PR at 43b132f5f87000d0452f23f2651a5efd8bcee221.

No blocking findings.

The ZBytes change makes the existing double-checked lazy materialization valid on the JVM: the volatile write publishes the copied array contents to fast-path readers, while the synchronized path still ensures only one thread consumes and closes the native handle. Moving the cache from a private constructor property to an initialized body property does not change the public API.

ReplyKeyExpr now matches the established ANY, MATCHING_QUERY order from main, while the explicit value field and fromInt/JNI conversion preserve the flat wire mapping independently of ordinal. The added test appropriately pins the consumer-visible order. The indentation and test-documentation cleanups are correct.

All current CI checks pass, including the JVM suites on Ubuntu and macOS.

— Codex (GPT-5)

[milyin]

Concurrency analysis: why this is a zenoh-kotlin ZBytes fix

The fix is correct, but the callback-to-Channel example is slightly imprecise. The actual defect is the double-checked cache inside the public zenoh-kotlin wrapper:

private var eager: ByteArray?

get() = eager ?: synchronized(this) {
    eager ?: run {
        val b = handle!!.toBytes(...)
        eager = b
        ...
        b
    }
}

Thread A writes eager while holding the monitor, but thread B reads it on the fast path without acquiring that monitor. Without @Volatile, there is no guaranteed happens-before relationship between those operations. A reader may observe the published reference without being guaranteed visibility of the preceding byte-array initialization. The volatile write/read pair makes the double-checked pattern valid:

native byte copy
    -> volatile write of eager
    -> volatile read by another thread
    -> copied contents are visible

Why this relates specifically to ZBytes

It is not because only payloads cross threads. Samples, queries, and other SDK objects cross threads too. ZBytes is currently unique because it introduces a two-representation lazy state machine:

received native handle
        -> first access copies bytes and closes the handle
        -> later accesses use an unlocked ByteArray cache

Other values are generally either built eagerly as Kotlin values or retained as generated native handles whose operations use synchronized handle locking. In the current public layer, ZBytes is the only wrapper using this unsynchronized-fast-path/synchronized-initialization pattern. The rule is general, though: any future wrapper using the same pattern would require equivalent safe publication.

Why the fix belongs in the external SDK layer

There are two distinct objects:

io.zenoh.jni.bytes.ZBytes       generated native handle
                -> wrapped by
io.zenoh.bytes.ZBytes           public SDK value + lazy ByteArray cache

The public zenoh-kotlin wrapper chooses to retain the handle, defer the copy, cache the array, close the handle after extraction, and serve subsequent public operations from that cache. toBytes(), string conversion, equality, hashing, and outgoing publication all eventually use it. The layer that creates this shared cached state is therefore the layer that must publish it safely. Moving the private constructor property to a volatile body property changes no public source or binary API.

Why zenoh-flat-jni does not already solve it

The generated handle protects a different boundary. It keeps the native pointer volatile, synchronizes close() and take(), and locks the handle around toBytes(). Those guarantees prevent native handle lifecycle races and ensure an individual JNI call returns a completed ByteArray.

They cannot establish visibility for io.zenoh.bytes.ZBytes.eager, because that downstream cache does not exist in zenoh-flat-jni. Volatility attaches to a field, not to an object returned from a method, and the generated handle volatile ptr is unrelated to publication of the wrapper cache.

The generated layer could avoid this only through a broader semantic change, such as eagerly copying every callback payload or moving caching into generated handles. That would undermine the deliberately lazy hot-path policy and could reintroduce its measured throughput cost.

Qualification

A single callback-to-Channel handoff followed by access from one consumer normally safely publishes the object and does not, by itself, constitute this race. The problematic case is the same ZBytes instance being accessed by multiple threads around or after lazy initialization without another synchronization edge. The channel path explains why these objects naturally escape the callback thread, but concurrent access is the condition that makes the defective double-check observable.

The fix also does not make the returned ByteArray immutable. Concurrent caller mutation of that array remains outside its scope.

— Codex (GPT-5)