diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index ab4f43704..b63bb4f0d 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -555,8 +555,9 @@ jobs:
# The gate reads a real archive with otool, so it runs where the iOS
# library is built: in a release, on all three archives, and in the
# Swift Package job below, on the simulator archive of every pull
- # request. This drives everything in it that is text handling, with a
- # stand-in for otool, on a runner that has none.
+ # request. This drives everything in it that is text handling, and the
+ # packaging function the gate sits inside, with stand-ins for otool,
+ # lipo and xcodebuild, on a runner that has none of them.
- name: Test the iOS deployment-target gate
run: bash scripts/tests/test-ios-min-os.sh
@@ -821,7 +822,12 @@ jobs:
- uses: Swatinem/rust-cache@v2
with:
- shared-key: ci-swift-package
+ # The deployment target is the podspec's, and cargo does not rebuild
+ # a Rust crate when IPHONEOS_DEPLOYMENT_TARGET changes. Without the
+ # hash a lowered target restores objects stamped for the old one, and
+ # the minimum-OS gate fails on them with no cargo clean to reach. It
+ # goes in shared-key: rust-cache ignores `key` when shared-key is set.
+ shared-key: ci-swift-package-${{ hashFiles('bindings/react-native/MeshSdk.podspec') }}
# One simulator slice, for the architecture of this runner. The release
# builds the device slice and both simulator architectures.
@@ -989,6 +995,13 @@ jobs:
:offline-protocol-android:testDebugUnitTest
:offline-protocol-android:publishAllPublicationsToStagingRepository
+ # A call to an API newer than minSdk passes every JVM test and throws on
+ # an older phone. Lint's NewApi check is the only thing that sees the
+ # class, and the library build runs it alone (see its lint block).
+ - name: Lint the library for calls newer than minSdk
+ working-directory: bindings/kotlin
+ run: gradle --no-daemon --stacktrace -PVERSION_NAME=0.0.0 :offline-protocol-android:lintDebug
+
# Gradle succeeds on a test task that found no test. The sources are
# read from another directory, so that is one wrong path away.
- name: Check that the suite ran
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 6bdfae3bd..1b9aad45e 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -112,7 +112,12 @@ jobs:
- uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2
with:
- shared-key: release-ios
+ # The deployment target is the podspec's, and cargo does not rebuild
+ # a Rust crate when IPHONEOS_DEPLOYMENT_TARGET changes. Without the
+ # hash a lowered target restores objects stamped for the old one, and
+ # the minimum-OS gate fails on them with no cargo clean to reach. It
+ # goes in shared-key: rust-cache ignores `key` when shared-key is set.
+ shared-key: release-ios-${{ hashFiles('bindings/react-native/MeshSdk.podspec') }}
- name: Cache uniffi-bindgen
id: cache-uniffi
@@ -1112,24 +1117,48 @@ jobs:
set -euo pipefail
PKG="@offline-protocol/mesh-sdk@$VERSION"
+ # A version that is not there yet prints nothing, or an E404. An npm
+ # error of any other kind (a DNS failure, a bad .npmrc, a registry
+ # outage) is not staging, and reading it as staging would spend the
+ # whole budget and then blame the registry. So each poll keeps what
+ # npm said, and the failure below prints the last error. Warnings
+ # (`npm warn`) are not errors and read as staging.
+ ERR="$(mktemp)"
+ LAST_ERROR=""
visible=false
for attempt in $(seq 1 "$VISIBLE_POLLS"); do
- if [ "$(npm view "$PKG" version 2>/dev/null || true)" = "$VERSION" ]; then
+ SEEN="$(npm view "$PKG" version 2>"$ERR" || true)"
+ if [ "$SEEN" = "$VERSION" ]; then
visible=true
echo "$PKG is readable on the registry (poll $attempt of $VISIBLE_POLLS)."
break
fi
- echo "$PKG not readable yet, probably still staging (poll $attempt of $VISIBLE_POLLS)."
+ if ! grep -qE '^npm (ERR!|error)' "$ERR" || grep -q 'E404' "$ERR"; then
+ # A missing version after an error means the error passed.
+ LAST_ERROR=""
+ echo "$PKG not readable yet, probably still staging (poll $attempt of $VISIBLE_POLLS)."
+ else
+ # One line: a workflow annotation ends at the first newline.
+ # `%` would start an escape in a workflow command.
+ LAST_ERROR="$(grep -E '^npm (ERR!|error)' "$ERR" | tail -n 5 | tr '\n' ' ' | sed 's/%/%25/g')"
+ echo "npm view failed, not a staging answer (poll $attempt of $VISIBLE_POLLS): $LAST_ERROR"
+ fi
sleep "$POLL_SECONDS"
done
if [ "$visible" != true ]; then
- echo "::error title=Published version not visible::$PKG was accepted by the \
+ if [ -n "$LAST_ERROR" ]; then
+ echo "::error title=npm view failed::$PKG could not be read back for \
+ $((VISIBLE_POLLS * POLL_SECONDS)) seconds, and npm reported an error rather than \
+ a missing version, so this may not be staging at all. Last error: $LAST_ERROR"
+ else
+ echo "::error title=Published version not visible::$PKG was accepted by the \
publish step but is still not readable after $((VISIBLE_POLLS * POLL_SECONDS)) \
seconds. The registry stages large uploads and can take this long; check \
'npm view $PKG dist.attestations' by hand before doing anything else. A re-run \
is safe (the publish step skips a version that is already there) but only \
waits again."
+ fi
exit 1
fi
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cd68c6de7..bfb8a09aa 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -70,8 +70,10 @@ archived by series under [docs/changelog/](docs/changelog/); see the
record proves the key; a damaged check over an empty store fails with
`FileStoreError::KeyCheckUnverifiable`, naming the file to remove. The
sealed store never deletes a record on a read; a damaged one is reported
- as `CorruptedData` naming its file, and one the listing skips is logged,
- once per store, as a warning naming its file. Each store holds an
+ as `CorruptedData` naming its file, and one the listing skips is logged
+ as a warning naming its file, once until that file is written or removed.
+ A record that cannot be read is reported as a read failure, never as one
+ sealed under another key, and never counts towards refusing the store key. Each store holds an
exclusive lock on its directory while open, so a second engine over the
same directories fails with `FileStoreError::InUse` instead of silently
diverging the MLS state. `close()` on either store releases its directory
@@ -120,8 +122,9 @@ archived by series under [docs/changelog/](docs/changelog/); see the
failed nor a first write that failed can disarm it); a state root that
holds records and belongs with another MLS store (an identity is in
place, but not the one that wrote the state), decided by the pairing ids
- the two stores' first open wrote, and for a state root never bound by
- whether its sealed records open under this MLS store's record key; an
+ the two stores' first open wrote, and for a state root never bound, or
+ bound to another id, by whether its sealed records open under this MLS
+ store's record key, which only this identity's do; an
account directory that cannot be read, which is never taken for empty;
and roots that are one directory
or one inside the other, compared as spelled before anything is created
@@ -171,15 +174,26 @@ archived by series under [docs/changelog/](docs/changelog/); see the
### Fixed
+- **The storage conformance suite no longer deletes a merging backend's
+ records.** `runStorageConformance` cleaned up its probe records by listing
+ a probe key type and deleting what it listed, before any check had run.
+ On a backend that merges key types, or lists them by the tail of the name,
+ that listing names real records too, and the cleanup deleted them. The
+ suite now checks key-type isolation first, with point writes and point
+ deletes only, and a failure ends the run there with that one check
+ reported. A run that passes it still has thirteen checks, under the same
+ names.
- **Relay timestamps parse on Android 7.** The Android bridge parsed them
with `java.time.Instant`, which exists from Android 8 (API 26), while the
SDK supports Android 7 (API 24). On Android 7 the call threw
`NoClassDefFoundError`, which the surrounding `catch (e: Exception)` does
not catch, for a legacy relay message and an ISO-8601 last-seen timestamp.
An application that enables core library desugaring was not affected. The
- timestamp is now parsed without `java.time`, with the answers `Instant`
- gave, and a Rust guard refuses a new `java.time` call in the bridge. Not
- run on an Android 7 device.
+ timestamp is now parsed without `java.time`, as RFC 3339, and gives the
+ answers `Instant` gave for every timestamp a relay sends. At the edges it
+ is stricter than `Instant`: hour 24, a fraction with no digits, lowercase
+ `t` or `z` and a leap second are refused. A Rust guard refuses a new
+ `java.time` call in the bridge. Not run on an Android 7 device.
- **An opted-in mesh wake with no wake service stops at once.** A sticky
restart that wakes JavaScript started the wake service by name, and
`startService` returns null rather than throwing when no such service is
diff --git a/bindings/kotlin/offline-protocol-android/build.gradle.kts b/bindings/kotlin/offline-protocol-android/build.gradle.kts
index 9a1490d27..552b32b93 100644
--- a/bindings/kotlin/offline-protocol-android/build.gradle.kts
+++ b/bindings/kotlin/offline-protocol-android/build.gradle.kts
@@ -117,6 +117,19 @@ android {
withSourcesJar()
}
}
+
+ // A call to an API newer than minSdk compiles, passes every unit test on
+ // the JVM, and throws NoClassDefFoundError on an older phone, which a
+ // `catch (e: Exception)` does not catch. java.time on Android 7 was one.
+ // Lint's NewApi check is the class of that defect, so it runs alone and
+ // fails the build: the other checks report findings this library has not
+ // decided on. lint.xml leaves out the generated bindings, which guard
+ // their one such call behind a Class.forName.
+ lint {
+ checkOnly += "NewApi"
+ abortOnError = true
+ lintConfig = file("lint.xml")
+ }
}
// A source set is a list of directories and cannot leave a file out, so the
diff --git a/bindings/kotlin/offline-protocol-android/lint.xml b/bindings/kotlin/offline-protocol-android/lint.xml
new file mode 100644
index 000000000..058567493
--- /dev/null
+++ b/bindings/kotlin/offline-protocol-android/lint.xml
@@ -0,0 +1,11 @@
+
+
+
+
+
+
+
diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/MeshForegroundService.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/MeshForegroundService.kt
index 46b56011b..499881398 100644
--- a/bindings/react-native/android/src/main/java/com/offlineprotocol/MeshForegroundService.kt
+++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/MeshForegroundService.kt
@@ -5,6 +5,7 @@ import android.app.NotificationChannel
import android.app.NotificationManager
import android.app.PendingIntent
import android.app.Service
+import android.content.ComponentName
import android.content.Context
import android.content.Intent
import android.content.pm.PackageManager
@@ -191,6 +192,14 @@ class MeshForegroundService : Service() {
/** The armed watchdog, or null when no wake is outstanding. */
private var wakeWatchdog: Runnable? = null
+ /**
+ * How [dispatchWake] starts the wake service: [startService], and a test
+ * seam only. `startService` answers null when it starts nothing, and
+ * Robolectric's never does, so the null branch below is reachable from a
+ * test only through here.
+ */
+ internal var startWakeService: (Intent) -> ComponentName? = { startService(it) }
+
inner class LocalBinder : Binder() {
fun getService(): MeshForegroundService = this@MeshForegroundService
}
@@ -396,7 +405,7 @@ class MeshForegroundService : Service() {
"declared in this application; stopping instead",
)
false
- } else if (startService(intent) == null) {
+ } else if (startWakeService(intent) == null) {
Log.w(TAG, "The mesh wake service did not start; stopping instead")
false
} else {
diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayTimestamps.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayTimestamps.kt
index 3c1f0252b..6083709c9 100644
--- a/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayTimestamps.kt
+++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayTimestamps.kt
@@ -11,12 +11,17 @@ package com.offlineprotocol
* API 24 or 25 the call throws `NoClassDefFoundError`, which is an `Error`
* and passes through a `catch (e: Exception)`. Desugaring would have to be
* switched on by every application, not by this library, so the parse is
- * written out here and behaves the same on every API level. It answers what
- * `Instant.parse` answered for every input the relay sends.
+ * written out here and behaves the same on every API level.
*
- * Stricter than the Swift twin, as `Instant` was: Foundation's formatter
- * rolls 2023-02-29 over into March and accepts 24:00 and a `+0530` offset,
- * and this refuses all three.
+ * It reads RFC 3339 internet date-time and answers what `Instant.parse`
+ * answered for every timestamp the relay sends. It is not `Instant` at the
+ * edges. It refuses hour 24 and a `.` with no digits after it, which RFC 3339
+ * refuses too, and lowercase `t` or `z` and a leap second `:60`, which RFC
+ * 3339 allows. `Instant.parse` accepted all four. No relay sends any of them.
+ *
+ * Stricter than the Swift twin: Foundation's formatter rolls 2023-02-29 over
+ * into March and accepts 24:00 and a `+0530` offset, and this refuses all
+ * three.
*/
object RelayTimestamps {
/**
@@ -30,9 +35,13 @@ object RelayTimestamps {
* RFC 3339 internet date-time, what the Swift twin's
* `ISO8601DateFormatter` accepts with `.withInternetDateTime`: a `T`, an
* optional fraction, and `Z` or a numeric offset.
+ *
+ * `[0-9]`, never `\d`: on Android `\d` is ICU's and matches every Unicode
+ * decimal digit, so `٢٠٢٤` would match here and then parse as 2024. The
+ * JVM the unit tests run on reads `\d` as ASCII and cannot tell.
*/
private val INTERNET_DATE_TIME = Regex(
- """(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2})(?:\.(\d{1,9}))?(Z|([+-])(\d{2}):(\d{2}))"""
+ """([0-9]{4})-([0-9]{2})-([0-9]{2})T([0-9]{2}):([0-9]{2}):([0-9]{2})(?:\.([0-9]{1,9}))?(Z|([+-])([0-9]{2}):([0-9]{2}))"""
)
fun parseToMsOrNull(timestampStr: String): Long? {
diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/WifiDirectManager.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/WifiDirectManager.kt
index 84f03eb1d..2226a82de 100644
--- a/bindings/react-native/android/src/main/java/com/offlineprotocol/WifiDirectManager.kt
+++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/WifiDirectManager.kt
@@ -584,8 +584,21 @@ class WifiDirectManager(
if (connected && hasRequiredPermissions()) {
wifiP2pManager?.requestConnectionInfo(channel) { info ->
info?.let {
+ val previousOwner = groupOwnerAddress
isGroupOwner = it.isGroupOwner
groupOwnerAddress = it.groupOwnerAddress?.hostAddress
+ // A new connection starts its redial ladder from the
+ // bottom. The delay doubles on each failed dial and was
+ // reset only by a proved stream or stop(), so a new group's
+ // first failed dial waited out the old group's backoff.
+ // Group owners on Android nearly always take the same
+ // address, so this compares against the null the
+ // disconnect branch below leaves, not one owner with
+ // another: it resets on every connection after a
+ // disconnect, the same group rejoined included.
+ if (groupOwnerAddress != previousOwner) {
+ reconnectDelayMs.set(RECONNECT_INITIAL_DELAY_MS)
+ }
emitDiagnostic("info", "Connection info", mapOf(
"isGroupOwner" to isGroupOwner,
diff --git a/bindings/react-native/android/src/test/java/com/offlineprotocol/MeshForegroundServiceTest.kt b/bindings/react-native/android/src/test/java/com/offlineprotocol/MeshForegroundServiceTest.kt
index 4b3499024..51fff1919 100644
--- a/bindings/react-native/android/src/test/java/com/offlineprotocol/MeshForegroundServiceTest.kt
+++ b/bindings/react-native/android/src/test/java/com/offlineprotocol/MeshForegroundServiceTest.kt
@@ -416,6 +416,23 @@ class MeshForegroundServiceTest {
assertTrue(shadowOf(service).isStoppedBySelf)
}
+ @Test
+ fun `a wake service that resolves but does not start stops at once`() {
+ // The service is declared, so it resolves, and the platform still
+ // starts nothing: `startService` answers null. Robolectric's never
+ // does, so the start goes through the seam.
+ optInToWake()
+ declareWakeService()
+
+ val service = createService()
+ service.startWakeService = { null }
+ val result = service.onStartCommand(null, 0, 1)
+
+ assertEquals(Service.START_NOT_STICKY, result)
+ assertTrue(shadowOf(service).isForegroundStopped)
+ assertTrue(shadowOf(service).isStoppedBySelf)
+ }
+
@Test
fun `the wake watchdog stops the keep-alive when no host arrives`() {
optInToWake()
diff --git a/bindings/react-native/android/src/test/java/com/offlineprotocol/RelayTimestampsTest.kt b/bindings/react-native/android/src/test/java/com/offlineprotocol/RelayTimestampsTest.kt
index 64837c9db..0c5dbf7f0 100644
--- a/bindings/react-native/android/src/test/java/com/offlineprotocol/RelayTimestampsTest.kt
+++ b/bindings/react-native/android/src/test/java/com/offlineprotocol/RelayTimestampsTest.kt
@@ -88,6 +88,38 @@ class RelayTimestampsTest {
assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00+0530"))
}
+ /** Each field's own bound, one at a time: every other field is valid. */
+ @Test
+ fun refusesAFieldOutOfItsRange() {
+ assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:60:00Z"))
+ assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:60Z"))
+ assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00+24:00"))
+ assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00+05:60"))
+ // The largest value of each is still read.
+ assertEquals(
+ 1704153599000L,
+ RelayTimestamps.parseToMsOrNull("2024-01-01T23:59:59Z")
+ )
+ assertEquals(
+ 1704067200000L - (23 * 3600L + 59 * 60L) * 1000L,
+ RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00+23:59")
+ )
+ }
+
+ /**
+ * Where this parts from `Instant.parse`, which accepted each of these. RFC 3339
+ * refuses hour 24 and an empty fraction, and allows lowercase `t`/`z` and
+ * a leap second; this refuses all of them, and no relay sends any.
+ */
+ @Test
+ fun refusesWhatInstantAcceptedAndNoRelaySends() {
+ assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T24:00:00Z"))
+ assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00.Z"))
+ assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01t00:00:00Z"))
+ assertNull(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00z"))
+ assertNull(RelayTimestamps.parseToMsOrNull("2016-12-31T23:59:60Z"))
+ }
+
@Test
fun epochSecondsAreScaledToMilliseconds() {
// ~2024-07 as epoch seconds; without the heuristic this would render
diff --git a/bindings/react-native/ios/WifiDirectManager.swift b/bindings/react-native/ios/WifiDirectManager.swift
index c49fb90fe..afb9b7176 100644
--- a/bindings/react-native/ios/WifiDirectManager.swift
+++ b/bindings/react-native/ios/WifiDirectManager.swift
@@ -274,6 +274,16 @@ public class WifiDirectManager: NSObject, TransportManager {
advertiser?.stopAdvertisingPeer()
advertiser = nil
+ // The session is forgotten first. The state-change and data callbacks
+ // check `self.session === session` on linkQueue, so from here a .connected
+ // that was already queued finds nothing to attach to. Cleared after
+ // endAll(), such a callback created a link in the just-emptied table,
+ // start() never clears it, and a remote that kept its MCPeerID met a
+ // stale refused link after a restart: no preamble, a refusal at its
+ // deadline, and one wasted round before the disconnect cleared it.
+ let old = session
+ session = nil
+
// Report every proved peer lost while the core still holds its link,
// on linkQueue so no delivery lands after a loss report. The session's
// own .notConnected callbacks then find nothing left to report.
@@ -281,9 +291,7 @@ public class WifiDirectManager: NSObject, TransportManager {
peers.endAll()
}
- // Disconnect session
- session?.disconnect()
- session = nil
+ old?.disconnect()
// Notify protocol
try? protocolInstance.wifiDirectStatusChanged(isConnected: false)
diff --git a/bindings/react-native/ios/tests/RelayTimestampsTests.swift b/bindings/react-native/ios/tests/RelayTimestampsTests.swift
index 040c40900..331f500e5 100644
--- a/bindings/react-native/ios/tests/RelayTimestampsTests.swift
+++ b/bindings/react-native/ios/tests/RelayTimestampsTests.swift
@@ -50,7 +50,7 @@ final class RelayTimestampsTests: XCTestCase {
// No zone: a local time, which names no instant. The Kotlin twin also
// refuses an impossible date, 24:00 and an offset without a colon, which
- // Foundation's formatter accepts. That difference predates both suites.
+ // Foundation's formatter accepts. No relay sends any of the three.
func testRefusesATimeWithNoZone() {
XCTAssertNil(RelayTimestamps.parseToMsOrNull("2024-01-01T00:00:00"))
}
diff --git a/bindings/react-native/scripts/build-uniffi-ios.sh b/bindings/react-native/scripts/build-uniffi-ios.sh
index ba26d2588..3e0a81299 100755
--- a/bindings/react-native/scripts/build-uniffi-ios.sh
+++ b/bindings/react-native/scripts/build-uniffi-ios.sh
@@ -61,12 +61,7 @@ static_lib_for() {
fi
}
-echo "Checking the deployment target of every object..."
-assert_archive_min_os "$(static_lib_for aarch64-apple-ios)" "$IOS_DEPLOYMENT_TARGET"
-assert_archive_min_os "$(static_lib_for aarch64-apple-ios-sim)" \
- "$(arm64_simulator_ceiling "$IOS_DEPLOYMENT_TARGET")"
-assert_archive_min_os "$(static_lib_for x86_64-apple-ios)" "$IOS_DEPLOYMENT_TARGET"
-
+# Packaging checks every archive against the deployment target first.
echo "Packaging the XCFramework..."
# Why an XCFramework, and why both slices share one archive basename: see
@@ -75,7 +70,8 @@ package_xcframework \
"$OUTPUT_DIR" \
"$(static_lib_for aarch64-apple-ios)" \
"$(static_lib_for aarch64-apple-ios-sim)" \
- "$(static_lib_for x86_64-apple-ios)"
+ "$(static_lib_for x86_64-apple-ios)" \
+ "$IOS_DEPLOYMENT_TARGET"
# Generate bindings — all languages, not just Swift: they are one artifact set
# off one UDL (see scripts/generate-bindings.sh).
diff --git a/bindings/react-native/scripts/shared/xcframework.sh b/bindings/react-native/scripts/shared/xcframework.sh
index 3d35b1d4e..e3aa6f8e3 100644
--- a/bindings/react-native/scripts/shared/xcframework.sh
+++ b/bindings/react-native/scripts/shared/xcframework.sh
@@ -202,15 +202,36 @@ assert_archive_min_os() {
esac
}
-# package_xcframework
+# package_xcframework
#
-# Stages the device archive and a fat simulator archive under one shared
-# basename, then builds /offline_protocol_uniffi.xcframework.
+# Checks every archive against the deployment target, then stages the device
+# archive and a fat simulator archive under one shared basename and builds
+# /offline_protocol_uniffi.xcframework.
+#
+# The gate is here, not beside the call, so the pod's XCFramework cannot be
+# packaged without it: a gate the caller has to remember was pinned by the
+# order of lines in the build script, and most edits that broke it kept the
+# order. scripts/package-swiftpm-xcframework.sh, which packages the Swift
+# package's XCFramework, does not come through here; the Swift Package CI
+# job calls assert_archive_min_os on its archive itself.
+# scripts/tests/test-ios-min-os.sh drives this function with stand-ins for
+# otool, lipo and xcodebuild.
package_xcframework() {
local output_dir="$1"
local device_lib="$2"
local sim_arm64_lib="$3"
local sim_x86_64_lib="$4"
+ local deployment_target="${5:-}"
+
+ if [ -z "$deployment_target" ]; then
+ echo "ERROR: package_xcframework needs the deployment target to check the archives against" >&2
+ return 1
+ fi
+
+ echo "Checking the deployment target of every object..."
+ assert_archive_min_os "$device_lib" "$deployment_target" || return 1
+ assert_archive_min_os "$sim_arm64_lib" "$(arm64_simulator_ceiling "$deployment_target")" || return 1
+ assert_archive_min_os "$sim_x86_64_lib" "$deployment_target" || return 1
local xcframework="$output_dir/offline_protocol_uniffi.xcframework"
diff --git a/crates/offline-protocol-leaf/src/shared.rs b/crates/offline-protocol-leaf/src/shared.rs
index be13d8444..98706fd27 100644
--- a/crates/offline-protocol-leaf/src/shared.rs
+++ b/crates/offline-protocol-leaf/src/shared.rs
@@ -18,6 +18,12 @@
//! glob over `alloc`, and any rename of `alloc`. The `embedded-core` CI job
//! refuses a direct import on the targets.
//!
+//! The scan reads one file's tokens and cannot see three routes: an import
+//! through a module of this crate that re-exports `alloc`, a glob over such a
+//! module, and a path a macro assembles. None of them is written in this
+//! crate, and a new one would be, visibly, a module that re-exports `alloc`
+//! or a macro that pastes a path together.
+//!
//! # Building a handle
//!
//! [`shared_store`] is the portable way. The replacement pointer cannot turn
@@ -388,6 +394,10 @@ mod tests {
"use crate::alloc as heap;",
"extern crate alloc as heap;",
"pub(crate) use alloc as mem;",
+ // A raw identifier is the same name.
+ "use alloc::r#sync::Arc;",
+ // A raw byte string ends at its quote and hashes, not the first quote.
+ "const B: &[u8] = br#\"a \" b\"#; use alloc::sync::Arc;",
] {
assert!(!counted_pointer_paths(code).is_empty(), "missed: {code}");
}
@@ -406,6 +416,10 @@ mod tests {
"mod sync { pub struct Arc; } use self::sync::Arc;",
"extern crate alloc; extern crate alloc as _;",
"let c = '\\''; let d = '\\\\'; let e = b'\\\\';",
+ // An escaped quote does not end a string.
+ "const S: &str = \"\\\" use alloc::sync::Arc; \\\"\";",
+ // A block comment ends at its own close, not the first `*/`.
+ "/* a /* b */ use alloc::sync::Arc; */",
] {
assert_eq!(
counted_pointer_paths(code),
diff --git a/crates/offline-protocol-mls/src/storage_conformance.rs b/crates/offline-protocol-mls/src/storage_conformance.rs
index fa0754247..a1e16963d 100644
--- a/crates/offline-protocol-mls/src/storage_conformance.rs
+++ b/crates/offline-protocol-mls/src/storage_conformance.rs
@@ -49,6 +49,14 @@ pub const CONFORMANCE_KEY_TYPE_OTHER: &str = "mls_storage_conformance_probe_othe
/// untouched category is empty and not an error.
const CONFORMANCE_KEY_TYPE_UNUSED: &str = "mls_storage_conformance_probe_unused";
+/// A key type that ends with [`CONFORMANCE_KEY_TYPE`], the way the SDK's
+/// own key types are related: `key_package` ends `contact_key_package` and
+/// `openmls_key_package`, and `group_state` ends `openmls_group_state`. The
+/// two probe types above are related by prefix only, so a backend that lists
+/// by suffix passed every check and then listed every OpenMLS record as a
+/// group. The isolation check writes its listing probe here too.
+const CONFORMANCE_KEY_TYPE_SUFFIXED: &str = "suffixed_mls_storage_conformance_probe";
+
/// The id the isolation check writes under both probe key types. Nothing
/// else uses it, so a point write or delete of it cannot touch a real
/// record even on a backend that ignores the key type.
@@ -532,6 +540,9 @@ fn key_types_are_isolated(storage: &dyn MlsStorage) -> Result<(), String> {
.and_then(|()| {
storage.store(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_LISTING_PROBE_ID, b"x")
})
+ .and_then(|()| {
+ storage.store(CONFORMANCE_KEY_TYPE_SUFFIXED, ISOLATION_LISTING_PROBE_ID, b"x")
+ })
.map_err(|err| {
format!("store failed, so isolation could not be checked and nothing was deleted: {err}")
})?;
@@ -555,6 +566,7 @@ fn key_types_are_isolated(storage: &dyn MlsStorage) -> Result<(), String> {
(CONFORMANCE_KEY_TYPE, ISOLATION_PROBE_ID),
(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_PROBE_ID),
(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_LISTING_PROBE_ID),
+ (CONFORMANCE_KEY_TYPE_SUFFIXED, ISOLATION_LISTING_PROBE_ID),
] {
let _ = storage.delete(key_type, key_id);
}
@@ -736,6 +748,7 @@ mod tests {
AbsentIsAnError,
DeleteOfAbsentFails,
IgnoresKeyType,
+ ListsBySuffix,
ListReturnsFirstPageOnly,
ListOfUnknownTypeFails,
KeepsIdUpToFirstColon,
@@ -852,7 +865,12 @@ mod tests {
.lock()
.unwrap()
.keys()
- .filter(|(kt, _)| *kt == key_type)
+ .filter(|(kt, _)| match self.fault {
+ // Matching the tail of the name, which the SDK's own
+ // key types share (see CONFORMANCE_KEY_TYPE_SUFFIXED).
+ Fault::ListsBySuffix => kt.ends_with(key_type.as_str()),
+ _ => *kt == key_type,
+ })
.map(|(_, id)| id.clone())
.collect();
match self.fault {
@@ -899,6 +917,7 @@ mod tests {
// nothing, so each check is shown failing the defect it exists for.
let cases = [
(Fault::IgnoresKeyType, "key_types_are_separate_namespaces"),
+ (Fault::ListsBySuffix, "key_types_are_separate_namespaces"),
(Fault::CorruptsValues, "store_then_load"),
(Fault::StringTypedValues, "binary_values_round_trip"),
(Fault::EmptyReadsAsMissing, "empty_value_is_not_missing"),
@@ -964,6 +983,29 @@ mod tests {
.expect("the check failed");
assert!(failure.detail.contains("KeyNotFound"), "{}", failure.detail);
}
+ #[test]
+ fn a_backend_that_lists_by_suffix_stops_the_run_before_any_delete() {
+ // Listing one key type by the tail of its name reaches every type
+ // that ends the same way. The run stops at the isolation check, and
+ // a real record in such a type is untouched.
+ let storage = Faulty::new(Fault::ListsBySuffix);
+ storage
+ .store("prefixed_mls_storage_conformance_probe", "real", b"kept")
+ .unwrap();
+ let report = run(&storage);
+ assert!(report.passed.is_empty(), "{:?}", report.passed);
+ assert_eq!(
+ report.failures[0].check,
+ "key_types_are_separate_namespaces"
+ );
+ assert_eq!(
+ storage
+ .load("prefixed_mls_storage_conformance_probe", "real")
+ .unwrap(),
+ Some(b"kept".to_vec())
+ );
+ }
+
#[test]
fn a_backend_that_merges_key_types_stops_the_run_before_any_delete() {
// The isolation check runs first because every listed delete after
diff --git a/crates/offline-protocol-uniffi/src/lib.rs b/crates/offline-protocol-uniffi/src/lib.rs
index 4e60c11bd..e0b5d276a 100644
--- a/crates/offline-protocol-uniffi/src/lib.rs
+++ b/crates/offline-protocol-uniffi/src/lib.rs
@@ -15135,11 +15135,12 @@ mod tests {
///
/// `minSdk` is 24 and `java.time` exists from API 26. On API 24 or 25 a
/// call throws `NoClassDefFoundError`, an `Error`, which passes through
- /// the `catch (e: Exception)` these call sites are written with. Nothing
- /// else would say so: the unit suite runs on a JVM that has `java.time`,
- /// the build does not run Android lint, and desugaring would be every
+ /// the `catch (e: Exception)` these call sites are written with. The unit
+ /// suite runs on a JVM that has `java.time`, and desugaring would be every
/// application's switch to throw, not this library's. `RelayTimestamps`
- /// parses ISO-8601 by hand for exactly this reason.
+ /// parses ISO-8601 by hand for exactly this reason. The Android Library
+ /// CI job runs lint's NewApi check, which covers every newer API; this
+ /// guard keeps the one that already shipped from coming back without it.
#[test]
fn android_bridge_sources_never_call_java_time() {
let callers: Vec = rn_android_kotlin_sources()
@@ -15154,6 +15155,26 @@ mod tests {
);
}
+ /// The Android relay timestamp parser matches ASCII digits only.
+ ///
+ /// On Android a regex `\d` is ICU's and matches every Unicode decimal
+ /// digit, and `toInt()` then reads them as their values, so a timestamp
+ /// written in Arabic-Indic digits would parse. The unit suite runs on a
+ /// JVM, where `\d` is ASCII, so no Kotlin test can see the difference.
+ #[test]
+ fn android_relay_timestamps_match_ascii_digits_only() {
+ let code =
+ rn_source_code_only("android/src/main/java/com/offlineprotocol/RelayTimestamps.kt");
+ assert!(
+ code.contains("[0-9]{4}"),
+ "RelayTimestamps.kt: expected the date-time pattern spelled with [0-9]"
+ );
+ assert!(
+ !code.contains("\\d"),
+ "RelayTimestamps.kt: `\\d` is any Unicode digit on Android; use [0-9]"
+ );
+ }
+
/// Nothing that blocks on the network shares the thread `stop()` waits on.
///
/// A background caller's `runSync` wait is deliberately unbounded — it is
@@ -16670,6 +16691,43 @@ mod tests {
);
}
+ /// Two orderings in the peer-stream managers that no test can run.
+ ///
+ /// iOS `stop()` forgets the session before it ends every link. The
+ /// state-change and data callbacks check `self.session === session` on
+ /// the link queue, so a `.connected` already queued then finds nothing; forgotten after
+ /// `endAll()`, it created a link in the emptied table that `start()`
+ /// never clears, and a remote that kept its MCPeerID met a stale refused
+ /// link on the next session.
+ ///
+ /// Android resets the redial delay on a new connection after a
+ /// disconnect (group owners nearly always share one address, so the
+ /// comparison is with the null the disconnect leaves). The delay doubles
+ /// per failed dial, so without the reset a new group's first failed dial
+ /// waited out the previous group's backoff.
+ #[test]
+ fn react_native_peer_stream_managers_forget_the_session_first_and_reset_on_a_new_group() {
+ let swift = rn_source_code_only("ios/WifiDirectManager.swift");
+ assert!(
+ swift.contains(
+ "let old = session session = nil onLinkQueueSync { peers.endAll() } \
+ old?.disconnect()"
+ ),
+ "ios/WifiDirectManager.swift: stop() must clear the session before endAll()"
+ );
+ let kotlin =
+ rn_source_code_only("android/src/main/java/com/offlineprotocol/WifiDirectManager.kt");
+ assert!(
+ kotlin.contains(
+ "val previousOwner = groupOwnerAddress isGroupOwner = it.isGroupOwner \
+ groupOwnerAddress = it.groupOwnerAddress?.hostAddress \
+ if (groupOwnerAddress != previousOwner) { \
+ reconnectDelayMs.set(RECONNECT_INITIAL_DELAY_MS) }"
+ ),
+ "android/.../WifiDirectManager.kt: a new connection must reset the redial delay"
+ );
+ }
+
/// Wi-Fi Direct hands the core only an address a preamble proved.
///
/// Both managers used to pass a transport-level string, a TCP endpoint on
diff --git a/crates/offline-protocol-uniffi/src/offline_protocol.udl b/crates/offline-protocol-uniffi/src/offline_protocol.udl
index 8117af5d8..eaa701a3e 100644
--- a/crates/offline-protocol-uniffi/src/offline_protocol.udl
+++ b/crates/offline-protocol-uniffi/src/offline_protocol.udl
@@ -61,7 +61,10 @@ namespace offline_protocol {
// — swappability that cannot be verified is a promise, not a property.
//
// The suite writes only under its own probe key types and deletes
- // everything it wrote, so it is safe against a live store.
+ // everything it wrote. Key-type isolation is checked first, with point
+ // writes and deletes, and a failure ends the run before anything is
+ // listed and deleted, so a backend that merges key types keeps its real
+ // records. Probe a new backend on a scratch instance until it is green.
string run_storage_conformance(ProtocolStateStorageProvider storage);
};
@@ -1618,19 +1621,21 @@ interface OfflineProtocol {
// (the message names the path), a wrong key, or a key check that cannot
// prove the key. After both opened: protocol state that belongs with
// another MLS store. The first open of a pair binds the two with one
- // pairing id, and later opens compare the ids; when either store holds
- // no id, the state root is asked whether its sealed records open under
- // this MLS store's record key. That is another identity's state, and the engine deletes what its
- // restore reads of it, queued and parked messages among them. A refusal
+ // pairing id, and later opens compare the ids. When either store holds
+ // no id, or the ids differ, the state root is asked whether its sealed
+ // records open under this MLS store's record key, and one that opens
+ // admits it. State that belongs with another MLS store is refused
+ // because the engine deletes what its restore reads of it, queued and
+ // parked messages among them. A refusal
// after the MLS store opened leaves its directory, lock file and key
// check, and no record, so the first refusal above still applies to a
// retry.
//
// A damaged record key is not refused, on the launch that finds it or
- // on any launch after while the MLS root keeps its pairing file. Nothing opens the state sealed under it again, so
- // a refusal would save no record, and the engine's new key and its
- // settlement of that state as failed is how the application learns
- // which messages were lost. What that launch cannot delete stays sealed
+ // on any launch after while the MLS root keeps its pairing file.
+ // Nothing opens the state sealed under it again, so a refusal would save
+ // no record, and the engine's new key and its settlement of that state
+ // as failed is how the application learns which messages were lost. What that launch cannot delete stays sealed
// under the lost key, and the pairing ids, not those records, say the
// state is still this identity's.
//
diff --git a/crates/offline-protocol/src/file_store/mls.rs b/crates/offline-protocol/src/file_store/mls.rs
index 63d792bb2..6522144f6 100644
--- a/crates/offline-protocol/src/file_store/mls.rs
+++ b/crates/offline-protocol/src/file_store/mls.rs
@@ -122,6 +122,19 @@ enum Probe {
NoneOpened,
}
+/// What one record file turned out to be.
+///
+/// A file that could not be read is not a file that did not open. Folding
+/// the two together sent a permission or disk error to the operator as "does
+/// not open under this store key", and counted it towards refusing the key.
+enum Opened {
+ Record(String, String),
+ /// Removed between the listing and the read.
+ Absent,
+ /// Read, and does not authenticate under this key and name.
+ DoesNotOpen,
+}
+
impl SealedFileMlsStorage {
/// Opens (creating if needed) the store at `//`, sealed
/// under the key `provider` supplies.
@@ -346,16 +359,40 @@ impl SealedFileMlsStorage {
.map(Zeroizing::new)
}
- /// Opens one record file into its `(key_type, key_id)`, or `None` when it
- /// does not authenticate under this key and name. The value is not
+ /// Opens one record file into its `(key_type, key_id)`. An error is a
+ /// read that failed, never a record that does not open. The value is not
/// copied: the only plaintext is the zeroizing buffer, dropped here.
- fn open_record(&self, path: &Path, name: &str) -> Option<(String, String)> {
- let BoundedRead::Bytes(raw) = records::read_bounded(path, MAX_SEALED_BYTES).ok()? else {
- return None;
+ fn open_record(&self, path: &Path, name: &str) -> std::io::Result {
+ let raw = match records::read_bounded(path, MAX_SEALED_BYTES)? {
+ BoundedRead::Bytes(raw) => raw,
+ BoundedRead::Absent => return Ok(Opened::Absent),
+ BoundedRead::Oversized => return Ok(Opened::DoesNotOpen),
};
- let plain = self.open_sealed(name, &raw)?;
- let header = records::parse_header(&plain)?;
- Some((header.key_type, header.key_id))
+ let opened = self
+ .open_sealed(name, &raw)
+ .and_then(|plain| records::parse_header(&plain))
+ .map_or(Opened::DoesNotOpen, |header| {
+ Opened::Record(header.key_type, header.key_id)
+ });
+ Ok(opened)
+ }
+
+ /// Whether this is the first listing to warn about `path` since the file
+ /// there was last written or removed.
+ fn first_listing_warning(&self, path: &Path) -> bool {
+ self.listing_warned
+ .lock()
+ .unwrap_or_else(std::sync::PoisonError::into_inner)
+ .insert(path.to_path_buf())
+ }
+
+ /// Forgets that a listing warned about `path`, once the file there has
+ /// been written or removed: a record damaged again later is news.
+ fn forget_listing_warning(&self, path: &Path) {
+ self.listing_warned
+ .lock()
+ .unwrap_or_else(std::sync::PoisonError::into_inner)
+ .remove(path);
}
/// Proves the store key against the check file, minting the file on a
@@ -397,7 +434,7 @@ impl SealedFileMlsStorage {
// path. A check that does not open with no records to prove anything
// stays a refusal; there is nothing sealed to lose, and the operator
// can remove the check file to start the store afresh.
- match self.probe_existing_records().map_err(io)? {
+ match self.probe_existing_records()? {
Probe::Opened => {
if check_present {
tracing::warn!(
@@ -422,11 +459,23 @@ impl SealedFileMlsStorage {
records::write_atomic(&self.directory, &path, &sealed).map_err(io)
}
- fn probe_existing_records(&self) -> std::io::Result {
+ /// Tries up to [`KEY_CHECK_PROBE_LIMIT`] records against the store key.
+ ///
+ /// A record that cannot be read says nothing about the key, so it is
+ /// skipped rather than counted against it, and a readable record later in
+ /// the walk can still prove the key. Only when none opens is a read
+ /// failure the answer, naming the record: refusing the key as wrong over
+ /// a permission error would send the operator after the wrong fault.
+ fn probe_existing_records(&self) -> Result {
+ let directory_error = |source| FileStoreError::Io {
+ path: self.directory.clone(),
+ source,
+ };
let mut probe = Probe::NoRecords;
let mut tried = 0usize;
- for type_entry in std::fs::read_dir(&self.directory)? {
- let type_entry = type_entry?;
+ let mut unreadable: Option = None;
+ for type_entry in std::fs::read_dir(&self.directory).map_err(directory_error)? {
+ let type_entry = type_entry.map_err(directory_error)?;
let is_type_dir = type_entry
.file_name()
.to_str()
@@ -435,27 +484,48 @@ impl SealedFileMlsStorage {
if !is_type_dir {
continue;
}
+ let type_directory = type_entry.path();
records::for_each_entry(
- &type_entry.path(),
+ &type_directory,
ENTRY_PREFIX,
KEY_CHECK_PROBE_LIMIT,
|path, name| {
if matches!(probe, Probe::Opened) || tried >= KEY_CHECK_PROBE_LIMIT {
return;
}
- tried += 1;
- probe = if self.open_record(path, name).is_some() {
- Probe::Opened
- } else {
- Probe::NoneOpened
- };
+ match self.open_record(path, name) {
+ Ok(Opened::Record(..)) => {
+ tried += 1;
+ probe = Probe::Opened;
+ }
+ Ok(Opened::DoesNotOpen) => {
+ tried += 1;
+ probe = Probe::NoneOpened;
+ }
+ // Removed meanwhile: not a record.
+ Ok(Opened::Absent) => {}
+ Err(source) => {
+ unreadable.get_or_insert(FileStoreError::Io {
+ path: path.to_path_buf(),
+ source,
+ });
+ }
+ }
},
- )?;
+ )
+ .map_err(|source| FileStoreError::Io {
+ path: type_directory.clone(),
+ source,
+ })?;
if matches!(probe, Probe::Opened) || tried >= KEY_CHECK_PROBE_LIMIT {
break;
}
}
- Ok(probe)
+ match (probe, unreadable) {
+ (Probe::Opened, _) => Ok(Probe::Opened),
+ (_, Some(err)) => Err(err),
+ (probe, None) => Ok(probe),
+ }
}
}
@@ -495,8 +565,11 @@ impl MlsStorage for SealedFileMlsStorage {
records::private_mkdir(&directory)
.map_err(|err| StorageError::StoreFailed(err.to_string()))?;
self.swept.sweep_once(&directory);
- records::write_atomic(&directory, &directory.join(&name), &sealed)
- .map_err(|err| StorageError::StoreFailed(err.to_string()))
+ let path = directory.join(&name);
+ records::write_atomic(&directory, &path, &sealed)
+ .map_err(|err| StorageError::StoreFailed(err.to_string()))?;
+ self.forget_listing_warning(&path);
+ Ok(())
}
fn load(&self, key_type: &str, key_id: &str) -> StorageResult>> {
@@ -537,9 +610,10 @@ impl MlsStorage for SealedFileMlsStorage {
.lock
.open()
.ok_or_else(|| StorageError::DeleteFailed(self.closed()))?;
- records::remove(&self.entry_path(key_type, key_id))
- .map(|_| ())
- .map_err(|err| StorageError::DeleteFailed(err.to_string()))
+ let path = self.entry_path(key_type, key_id);
+ records::remove(&path).map_err(|err| StorageError::DeleteFailed(err.to_string()))?;
+ self.forget_listing_warning(&path);
+ Ok(())
}
fn list_keys(&self, key_type: &str) -> StorageResult> {
@@ -553,21 +627,30 @@ impl MlsStorage for SealedFileMlsStorage {
ENTRY_PREFIX,
records::MAX_LISTED_ENTRIES,
|path, name| match self.open_record(path, name) {
- Some((record_type, record_id)) if record_type == key_type => {
+ Ok(Opened::Record(record_type, record_id)) if record_type == key_type => {
keys.insert(record_id);
}
+ // Deleted since the directory was read: not a record.
+ Ok(Opened::Absent) => {}
+ // Left out of the listing, and named with the real cause. A
+ // listing that failed outright on one unreadable file would
+ // stop an engine from finding any of its groups.
+ Err(err) => {
+ if self.first_listing_warning(path) {
+ tracing::warn!(
+ path = %path.display(),
+ error = %err,
+ "sealed MLS record could not be read; left out of the listing"
+ );
+ }
+ }
// The listing is how the engine finds its groups, so a record
// skipped here is one the engine never learns it had. Same
// level and same path as `load` reports: names are keyed
// digests, and the path is the only way to find the file.
// Once per file per store: listings are frequent.
- _ => {
- let first = self
- .listing_warned
- .lock()
- .unwrap_or_else(std::sync::PoisonError::into_inner)
- .insert(path.to_path_buf());
- if first {
+ Ok(Opened::Record(..) | Opened::DoesNotOpen) => {
+ if self.first_listing_warning(path) {
tracing::warn!(
path = %path.display(),
"sealed MLS record does not open under this store key; \
@@ -956,6 +1039,184 @@ mod tests {
.contains(&damaged));
}
+ /// A record written or removed after a listing warned about it is a new
+ /// file: damaged again later, it is warned about again.
+ #[test]
+ fn a_record_rewritten_after_a_warning_is_warned_about_again() {
+ let root = TempRoot::new("mls-list-rewarn");
+ let store = open(&root, 1);
+ store.store("group_state", "a", b"one").expect("store");
+ let damaged = store.entry_path("group_state", "a");
+ let damage = || {
+ let mut bytes = std::fs::read(&damaged).expect("read");
+ bytes[SEALED_MAGIC.len() + NONCE_BYTES] ^= 1;
+ std::fs::write(&damaged, bytes).expect("write");
+ };
+ damage();
+
+ let _callsite = listing_warning_callsite();
+ let warnings = Warnings::default();
+ let _default = tracing::dispatcher::set_default(&tracing::Dispatch::new(warnings.clone()));
+ tracing::callsite::rebuild_interest_cache();
+ let path = damaged.display().to_string();
+ let named = || {
+ warnings
+ .0
+ .lock()
+ .expect("warnings")
+ .iter()
+ .filter(|w| w.contains(&path))
+ .count()
+ };
+
+ assert!(store.list_keys("group_state").expect("list").is_empty());
+ assert_eq!(named(), 1);
+
+ store.store("group_state", "a", b"again").expect("store");
+ damage();
+ assert!(store.list_keys("group_state").expect("list").is_empty());
+ assert_eq!(
+ named(),
+ 2,
+ "a rewritten record damaged again was not reported"
+ );
+
+ store.delete("group_state", "a").expect("delete");
+ assert!(
+ !store
+ .listing_warned
+ .lock()
+ .expect("memo")
+ .contains(&damaged),
+ "a removed record's warning is still remembered"
+ );
+ }
+
+ /// Makes `path` unreadable, or answers false where the test runs with
+ /// the power to read it anyway.
+ #[cfg(unix)]
+ fn make_unreadable(path: &Path) -> bool {
+ use std::os::unix::fs::PermissionsExt;
+ std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o000)).expect("chmod");
+ std::fs::read(path).is_err()
+ }
+
+ #[cfg(unix)]
+ fn make_readable(path: &Path) {
+ use std::os::unix::fs::PermissionsExt;
+ std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)).expect("chmod");
+ }
+
+ /// A record that cannot be read is not evidence against the store key.
+ /// Counted as one that does not open, a permission error on the only
+ /// record refused the right key as a wrong one.
+ #[cfg(unix)]
+ #[test]
+ fn an_unreadable_record_is_an_io_error_not_a_wrong_key() {
+ let root = TempRoot::new("mls-unreadable-probe");
+ let (check, record) = {
+ let store = open(&root, 1);
+ store.store("group_state", "a", b"one").expect("store");
+ (
+ store.directory().join(KEY_CHECK_FILE),
+ store.entry_path("group_state", "a"),
+ )
+ };
+ std::fs::remove_file(&check).expect("remove check");
+ if !make_unreadable(&record) {
+ return;
+ }
+ let outcome = SealedFileMlsStorage::open(root.path(), &namespace(), &key(1));
+ make_readable(&record);
+ match outcome {
+ Err(FileStoreError::Io { path, .. }) => {
+ assert_eq!(path, record, "the error does not name the record");
+ }
+ Err(other) => panic!("expected an I/O error naming the record, got {other}"),
+ Ok(_) => panic!("an unreadable record proved the key"),
+ }
+ assert!(
+ !check.exists(),
+ "no key check was minted on an unread record"
+ );
+ open(&root, 1);
+ assert!(check.exists(), "once readable, the record proves the key");
+ }
+
+ /// An unreadable record does not stand in the way of a readable one that
+ /// proves the key, whichever the directory lists first.
+ ///
+ /// File names are keyed digests, so the order the probe meets records in
+ /// is not the test's to choose. Four of five are unreadable, so a probe
+ /// that stopped at the first unreadable record fails here on most runs.
+ #[cfg(unix)]
+ #[test]
+ fn an_unreadable_record_does_not_hide_one_that_proves_the_key() {
+ let root = TempRoot::new("mls-unreadable-beside-good");
+ let ids = ["a", "b", "c", "d", "e"];
+ let (check, unreadable) = {
+ let store = open(&root, 1);
+ for id in ids {
+ store.store("group_state", id, b"value").expect("store");
+ }
+ (
+ store.directory().join(KEY_CHECK_FILE),
+ ids[1..]
+ .iter()
+ .map(|id| store.entry_path("group_state", id))
+ .collect::>(),
+ )
+ };
+ std::fs::remove_file(&check).expect("remove check");
+ if !unreadable.iter().all(|path| make_unreadable(path)) {
+ unreadable.iter().for_each(|path| make_readable(path));
+ return;
+ }
+ let outcome = SealedFileMlsStorage::open(root.path(), &namespace(), &key(1));
+ unreadable.iter().for_each(|path| make_readable(path));
+ outcome.expect("a readable record proves the key");
+ assert!(check.exists(), "the key check is rebuilt");
+ }
+
+ /// The listing names a read failure as one, not as a record sealed under
+ /// another key, and still returns the records it could read.
+ #[cfg(unix)]
+ #[test]
+ fn an_unreadable_record_is_listed_as_unreadable() {
+ let root = TempRoot::new("mls-unreadable-list");
+ let store = open(&root, 1);
+ store.store("group_state", "a", b"one").expect("store");
+ store.store("group_state", "b", b"two").expect("store");
+ let record = store.entry_path("group_state", "a");
+ if !make_unreadable(&record) {
+ return;
+ }
+
+ let _callsite = listing_warning_callsite();
+ let warnings = Warnings::default();
+ let _default = tracing::dispatcher::set_default(&tracing::Dispatch::new(warnings.clone()));
+ tracing::callsite::rebuild_interest_cache();
+
+ let listed = store.list_keys("group_state");
+ make_readable(&record);
+ assert_eq!(listed.expect("list"), vec!["b"]);
+ let path = record.display().to_string();
+ let said: Vec = warnings
+ .0
+ .lock()
+ .expect("warnings")
+ .iter()
+ .filter(|w| w.contains(&path))
+ .cloned()
+ .collect();
+ assert_eq!(said.len(), 1, "{said:?}");
+ assert!(
+ said[0].contains("could not be read") && !said[0].contains("store key"),
+ "the warning blames the key for a read failure: {}",
+ said[0]
+ );
+ }
+
/// Nothing to write down in a record's name, so the error must carry the
/// path: file names are keyed digests, and an operator cannot find the
/// damaged file any other way.
diff --git a/crates/offline-protocol/src/file_store/pair.rs b/crates/offline-protocol/src/file_store/pair.rs
index 958c2179d..10aed8f59 100644
--- a/crates/offline-protocol/src/file_store/pair.rs
+++ b/crates/offline-protocol/src/file_store/pair.rs
@@ -8,17 +8,25 @@
//! merely somewhere else.
//!
//! Ownership is recorded, not inferred. The first open of a pair writes one
-//! random pairing id into both stores, the MLS store first. A later open
-//! compares the two ids, and nothing the engine does to its record key
-//! changes them. Asking the sealed records instead
-//! ([`FileProtocolStateStorage::sealed_state`]) cannot tell another
-//! identity's state from this identity's state under a record key the engine
-//! had to regenerate: records sealed under the lost key outlive the launch
-//! that regenerated it (documents are read lazily, and each restore walk
-//! stops at its delete allowance), and the next launch finds records that do
-//! not open under a usable key. That reading refused the same identity over
-//! its own roots. It remains only as the fallback for a state store that was
-//! never bound.
+//! random pairing id into both stores, the protocol-state store first (see
+//! [`bind`] for why that order). A later open compares the two ids, and
+//! nothing the engine does to its record key changes them.
+//!
+//! The records alone ([`FileProtocolStateStorage::sealed_state`]) cannot
+//! tell another identity's state from this identity's state under a record
+//! key the engine had to regenerate: records sealed under the lost key
+//! outlive the launch that regenerated it (documents are read lazily, and
+//! each restore walk stops at its delete allowance), and the next launch
+//! finds records that do not open under a usable key. That reading refused
+//! the same identity over its own roots, so it decides only where the ids
+//! cannot: a store that holds no readable id, and ids that differ.
+//!
+//! Where the ids differ, only a sealed record that opens under this MLS
+//! store's record key admits the state, since only this identity's records
+//! open under it. That is the MLS store that lost its pairing file and was
+//! bound again beside another state root. The state root then takes the
+//! MLS store's id, so two state roots can carry one id; each still belongs
+//! to that identity.
use std::path::{Component, Path, PathBuf};
use std::sync::Arc;
@@ -164,8 +172,11 @@ impl FileStorePair {
/// Whether the state store belongs with the MLS store.
///
/// Decided by the pairing ids when both stores hold one: equal ids belong
-/// together, and a state store bound to another id belongs with another MLS
-/// store. When the MLS store holds no readable id (never bound, restored from
+/// together. A state store bound to another id belongs with another MLS
+/// store unless one of its sealed records opens under this MLS store's
+/// record key, which only this identity's records do: an MLS store that lost
+/// its pairing file and was bound again beside another state root still
+/// owns the state root it started with. When the MLS store holds no readable id (never bound, restored from
/// a backup older than its binding, the file removed or damaged), or the
/// state store holds none, the sealed records decide: another identity's
/// records still open under no key this store holds.
@@ -183,7 +194,15 @@ fn ownership(
) -> Result {
match (state.pairing()?, pairing) {
(Some(bound), StoredPairing::Present(ours)) if bound == *ours => Ok(SealedState::Opens),
- (Some(_), StoredPairing::Present(_)) => Ok(SealedState::Foreign),
+ // A record that opens is proof no id can overrule. Anything short of
+ // it stays another MLS store's: records that do not open, a damaged
+ // record key, and unsealed records alone, which prove nothing.
+ (Some(_), StoredPairing::Present(_)) => Ok(match state.sealed_state(secure)? {
+ SealedState::Opens => SealedState::Opens,
+ SealedState::Empty | SealedState::Foreign | SealedState::KeyDamaged => {
+ SealedState::Foreign
+ }
+ }),
(Some(_), StoredPairing::Absent | StoredPairing::Damaged) | (None, _) => {
state.sealed_state(secure)
}
@@ -393,7 +412,7 @@ mod tests {
}
}
- /// The first open binds both stores to one id, the MLS store first, and
+ /// The first open binds both stores to one id, the state store first, and
/// neither pairing file is a record: the probes that decide whether a new
/// identity may start still see an empty pair.
#[test]
@@ -554,6 +573,111 @@ mod tests {
open(&roots).expect("the next open admits the pair and binds it");
}
+ /// Gives the MLS store a record key and seals one record under it in the
+ /// state store, as the engine does.
+ fn seal_one_record(pair: &FileStorePair, key: [u8; 32]) {
+ use crate::protocol::storage_keys::{STATE_RECORD_KEY, STATE_RECORD_KEY_ID};
+ pair.secure
+ .store(STATE_RECORD_KEY, STATE_RECORD_KEY_ID, &key)
+ .expect("a record key");
+ let key_type = crate::protocol::sealed_state_key_types()
+ .next()
+ .expect("a sealed category");
+ let sealed = crate::protocol::state_crypto::StateRecordCipher::new(&key)
+ .seal(key_type, "parked", b"ours")
+ .expect("seal");
+ pair.state
+ .store(key_type, "parked", &sealed)
+ .expect("a sealed record");
+ }
+
+ /// An id mismatch is not the last word when a record opens. The MLS
+ /// store lost its pairing file and was bound again beside a fresh state
+ /// root; its first state root, put back, holds records only this
+ /// identity's record key opens, and it is admitted and bound again.
+ #[test]
+ fn a_state_root_whose_records_open_is_admitted_over_an_id_mismatch() {
+ let ours = roots("pair-mismatch-opens");
+ let pair = open(&ours).expect("open");
+ pair.secure
+ .store("identity", "self", b"ours")
+ .expect("an identity");
+ seal_one_record(&pair, [5; 32]);
+ let pairing_file = pair.secure.directory().join(PAIRING_FILE);
+ pair.close();
+ std::fs::remove_file(&pairing_file).expect("lose the MLS pairing file");
+
+ let elsewhere = TempRoot::new("pair-mismatch-elsewhere");
+ let rebound = FileStorePair::open(
+ ours.keys.path(),
+ elsewhere.path(),
+ &namespace(),
+ &StaticStoreKey::new([4; 32]),
+ )
+ .expect("bound beside a fresh state root");
+ let StoredPairing::Present(new_id) = rebound.secure.pairing().unwrap() else {
+ panic!("bound again");
+ };
+ rebound.close();
+ assert_ne!(
+ FileProtocolStateStorage::open(ours.state.path(), &namespace())
+ .and_then(|state| {
+ let id = state.pairing();
+ state.close();
+ id
+ })
+ .expect("the first state root"),
+ Some(new_id),
+ "the first state root still carries the old id"
+ );
+
+ let pair = open(&ours).expect("its own records open under its record key");
+ assert_eq!(pair.state.pairing().unwrap(), Some(new_id), "bound again");
+ }
+
+ /// A state store with no record is admitted beside an MLS store bound to
+ /// another id, and takes that id: the ids alone would call it another
+ /// MLS store's, and it has nothing to lose to anyone.
+ #[test]
+ fn a_state_store_with_no_record_is_rebound_beside_another_bound_mls_store() {
+ let theirs = roots("pair-empty-bound-theirs");
+ open(&theirs).expect("their pair").close();
+ let ours = roots("pair-empty-bound-ours");
+ open(&ours).expect("our pair").close();
+
+ let pair = FileStorePair::open(
+ theirs.keys.path(),
+ ours.state.path(),
+ &namespace(),
+ &StaticStoreKey::new([4; 32]),
+ )
+ .expect("no record to lose");
+ let StoredPairing::Present(id) = pair.secure.pairing().unwrap() else {
+ panic!("bound");
+ };
+ assert_eq!(pair.state.pairing().unwrap(), Some(id));
+ }
+
+ /// A reinstall: the state root is gone and made again beside an MLS root
+ /// that is still bound. The new state store takes the MLS store's id, or
+ /// every later open would judge the pair by its records alone.
+ #[test]
+ fn a_state_root_made_again_takes_the_bound_mls_id() {
+ let roots = roots("pair-reinstall");
+ let pair = open(&roots).expect("open");
+ pair.secure
+ .store("identity", "self", b"ours")
+ .expect("an identity");
+ let StoredPairing::Present(id) = pair.secure.pairing().unwrap() else {
+ panic!("bound");
+ };
+ pair.close();
+ std::fs::remove_dir_all(roots.state.path()).expect("remove the state root");
+
+ let pair = open(&roots).expect("reopen over a new state root");
+ assert_eq!(pair.state.pairing().unwrap(), Some(id));
+ }
+
/// A pairing file that no longer opens names no state store. The pair
/// is judged by its records, and bound again once it passes.
#[test]
diff --git a/crates/offline-protocol/src/protocol/tests/data_layer.rs b/crates/offline-protocol/src/protocol/tests/data_layer.rs
index f3a3dd035..bffd06653 100644
--- a/crates/offline-protocol/src/protocol/tests/data_layer.rs
+++ b/crates/offline-protocol/src/protocol/tests/data_layer.rs
@@ -743,6 +743,103 @@ fn the_conformance_suite_catches_a_broken_backend() {
);
}
+/// A backend whose key types bleed into each other, one of two ways.
+#[derive(Clone, Copy)]
+enum KeyTypeFault {
+ /// Every key type is one namespace.
+ Merged,
+ /// A listing matches the tail of the key type's name.
+ ListsBySuffix,
+}
+
+struct KeyTypeFaultStorage {
+ fault: KeyTypeFault,
+ records: Mutex>>,
+}
+
+impl KeyTypeFaultStorage {
+ fn new(fault: KeyTypeFault) -> Self {
+ Self {
+ fault,
+ records: Mutex::default(),
+ }
+ }
+
+ fn key(&self, key_type: &str, key_id: &str) -> (String, String) {
+ let key_type = match self.fault {
+ KeyTypeFault::Merged => "",
+ KeyTypeFault::ListsBySuffix => key_type,
+ };
+ (key_type.to_string(), key_id.to_string())
+ }
+}
+
+impl ProtocolStateStorage for KeyTypeFaultStorage {
+ fn store(&self, key_type: &str, key_id: &str, data: &[u8]) -> ProtocolStateResult<()> {
+ let key = self.key(key_type, key_id);
+ self.records
+ .lock()
+ .expect("lock")
+ .insert(key, data.to_vec());
+ Ok(())
+ }
+ fn load(&self, key_type: &str, key_id: &str) -> ProtocolStateResult>> {
+ let key = self.key(key_type, key_id);
+ Ok(self.records.lock().expect("lock").get(&key).cloned())
+ }
+ fn delete(&self, key_type: &str, key_id: &str) -> ProtocolStateResult<()> {
+ let key = self.key(key_type, key_id);
+ self.records.lock().expect("lock").remove(&key);
+ Ok(())
+ }
+ fn list_keys(&self, key_type: &str) -> ProtocolStateResult> {
+ let (wanted, _) = self.key(key_type, "");
+ Ok(self
+ .records
+ .lock()
+ .expect("lock")
+ .keys()
+ .filter(|(kt, _)| match self.fault {
+ KeyTypeFault::Merged => *kt == wanted,
+ KeyTypeFault::ListsBySuffix => kt.ends_with(wanted.as_str()),
+ })
+ .map(|(_, id)| id.clone())
+ .collect())
+ }
+}
+
+/// The suite is exposed over the FFI and run against real providers, so a
+/// backend that merges key types must not lose a record to it: listing a
+/// probe type names every record there, and the cleanup deletes what it
+/// lists. The isolation check runs first and ends the run before any
+/// listed delete. The same for a backend that lists by the tail of a name,
+/// which a real category ending like a probe type would meet.
+#[test]
+fn the_conformance_suite_stops_before_deleting_from_a_backend_whose_key_types_bleed() {
+ for (fault, real_type) in [
+ (KeyTypeFault::Merged, "outbox"),
+ (
+ KeyTypeFault::ListsBySuffix,
+ "prefixed_storage_conformance_probe",
+ ),
+ ] {
+ let storage = KeyTypeFaultStorage::new(fault);
+ storage.store(real_type, "parked", b"kept").expect("seed");
+ let report = crate::storage_conformance::run(&storage);
+ assert!(report.passed.is_empty(), "{:?}", report.passed);
+ assert_eq!(report.failures.len(), 1, "{}", report.summary());
+ assert_eq!(
+ report.failures[0].check,
+ "key_types_are_separate_namespaces"
+ );
+ assert_eq!(
+ storage.load(real_type, "parked").expect("load"),
+ Some(b"kept".to_vec()),
+ "{real_type}: the suite deleted a real record"
+ );
+ }
+}
+
#[test]
fn the_mls_storage_conformance_suite_is_reachable_from_the_engine() {
// The engine re-exports the MLS suite beside the state suite, so a Rust
diff --git a/crates/offline-protocol/src/storage_conformance.rs b/crates/offline-protocol/src/storage_conformance.rs
index c1d94a87d..b1b03048e 100644
--- a/crates/offline-protocol/src/storage_conformance.rs
+++ b/crates/offline-protocol/src/storage_conformance.rs
@@ -12,7 +12,16 @@
//! The suite drives the trait directly rather than going through the
//! sealing chokepoint, because what is under test is the adapter's contract:
//! bytes in, the same bytes out, addressed by `(key_type, key_id)`. It
-//! writes only under its own key type, and deletes everything it wrote.
+//! writes only under its own key types, and deletes everything it wrote.
+//!
+//! Key-type isolation is checked first, with point writes and point deletes
+//! of ids nothing else uses, and a failure ends the run there. Every listed
+//! delete after it trusts that listing a probe key type names only probe
+//! records; on a backend that merges key types it names real records too,
+//! and the cleanup would delete them. That makes the suite safe to run
+//! against a live store whatever the backend does with key types. It is not
+//! a promise for every defect: probe a new backend on a scratch instance
+//! until it is green.
use crate::protocol_state_storage::{
ProtocolStateError, ProtocolStateStorage, MAX_PROTOCOL_STATE_RECORD_TRANSFER_BYTES,
@@ -27,6 +36,20 @@ pub const CONFORMANCE_KEY_TYPE: &str = "storage_conformance_probe";
/// A second key type, used to prove categories do not bleed into each other.
pub const CONFORMANCE_KEY_TYPE_OTHER: &str = "storage_conformance_probe_other";
+/// A key type that ends with [`CONFORMANCE_KEY_TYPE`]. The second probe type
+/// is related to the first by prefix only, and a backend that lists by the
+/// tail of a name is caught only by one related by suffix.
+const CONFORMANCE_KEY_TYPE_SUFFIXED: &str = "suffixed_storage_conformance_probe";
+
+/// The id the isolation check writes under two probe key types. Nothing else
+/// uses it, so a point write or delete of it cannot reach a real record even
+/// on a backend that ignores the key type.
+const ISOLATION_PROBE_ID: &str = "storage_conformance_isolation";
+
+/// The id the isolation check writes under the other probe key types only,
+/// to catch a listing that crosses key types.
+const ISOLATION_LISTING_PROBE_ID: &str = "storage_conformance_isolation_listing";
+
/// The report types are the MLS suite's, re-exported, so the two suites
/// render one JSON shape and one reader serves both.
pub use offline_protocol_mls::storage_conformance::{ConformanceFailure, ConformanceReport};
@@ -56,18 +79,31 @@ fn expect_load(
pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport {
let mut report = ConformanceReport::new();
+ // 1. Key types are separate namespaces, checked before anything is
+ // deleted. The data layer keeps documents, their delta logs and space
+ // indexes in three of them; a backend that merges them would serve a
+ // delta where a document was asked for. It is also the one defect that
+ // turns this suite destructive (see the module doc), so a failure ends
+ // the run here.
+ let isolation = key_types_are_isolated(storage);
+ let isolated = isolation.is_ok();
+ report.check("key_types_are_separate_namespaces", isolation);
+ if !isolated {
+ return report;
+ }
+
// Anything left from an interrupted earlier run would make the listing
// checks report failures that are not the adapter's fault.
cleanup(storage);
- // 1. The basic contract.
+ // 2. The basic contract.
let outcome = storage
.store(CONFORMANCE_KEY_TYPE, "basic", b"hello")
.map_err(|err| format!("store failed: {err}"))
.and_then(|()| expect_load(storage, "basic", b"hello"));
report.check("store_then_load", outcome);
- // 2. Values are bytes, not text. A backend that round-trips through a
+ // 3. Values are bytes, not text. A backend that round-trips through a
// string type mangles these, and the failure is invisible until a
// sealed record (which is ciphertext) comes back wrong.
let binary: Vec = (0u8..=255).collect();
@@ -77,14 +113,14 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport {
.and_then(|()| expect_load(storage, "binary", &binary));
report.check("binary_values_round_trip", outcome);
- // 3. An empty value is a value, and must not read back as absent.
+ // 4. An empty value is a value, and must not read back as absent.
let outcome = storage
.store(CONFORMANCE_KEY_TYPE, "empty", b"")
.map_err(|err| format!("store failed: {err}"))
.and_then(|()| expect_load(storage, "empty", b""));
report.check("empty_value_is_not_missing", outcome);
- // 4. A second store replaces; it does not append or refuse.
+ // 5. A second store replaces; it does not append or refuse.
let outcome = storage
.store(CONFORMANCE_KEY_TYPE, "overwrite", b"first")
.and_then(|()| storage.store(CONFORMANCE_KEY_TYPE, "overwrite", b"second"))
@@ -92,7 +128,7 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport {
.and_then(|()| expect_load(storage, "overwrite", b"second"));
report.check("store_overwrites", outcome);
- // 5. A key that was never written reads as missing, not as an error.
+ // 6. A key that was never written reads as missing, not as an error.
let outcome = match storage.load(CONFORMANCE_KEY_TYPE, "never_written") {
Ok(None) => Ok(()),
Ok(Some(_)) => Err("a key that was never written returned a value".to_string()),
@@ -103,7 +139,7 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport {
};
report.check("absent_key_loads_as_none", outcome);
- // 6. Delete removes, and deleting again is not an error. The data layer
+ // 7. Delete removes, and deleting again is not an error. The data layer
// deletes folded delta records after a crash may already have taken
// them, so a delete that fails on an absent key turns a clean recovery
// into a spurious failure.
@@ -124,25 +160,12 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport {
);
report.check("delete_removes_and_is_idempotent", outcome);
- // 7. Key types are separate namespaces. The data layer keeps documents,
- // their delta logs and space indexes in three of them; a backend that
- // merges them would serve a delta where a document was asked for.
- let outcome = storage
- .store(CONFORMANCE_KEY_TYPE, "shared_id", b"mine")
- .and_then(|()| storage.store(CONFORMANCE_KEY_TYPE_OTHER, "shared_id", b"theirs"))
- .map_err(|err| format!("store failed: {err}"))
- .and_then(|()| expect_load(storage, "shared_id", b"mine"))
- .and_then(
- |()| match storage.load(CONFORMANCE_KEY_TYPE_OTHER, "shared_id") {
- Ok(Some(value)) if value == b"theirs" => Ok(()),
- Ok(_) => Err("the same key id in two key types collided".to_string()),
- Err(err) => Err(format!("load failed: {err}")),
- },
- );
- report.check("key_types_are_separate_namespaces", outcome);
-
// 8. Listing names what is there, and only within its own key type.
let outcome = (|| -> std::result::Result<(), String> {
+ storage
+ .store(CONFORMANCE_KEY_TYPE, "shared_id", b"mine")
+ .and_then(|()| storage.store(CONFORMANCE_KEY_TYPE_OTHER, "shared_id", b"theirs"))
+ .map_err(|err| format!("store failed: {err}"))?;
let keys = storage
.list_keys(CONFORMANCE_KEY_TYPE)
.map_err(|err| format!("list_keys failed: {err}"))?;
@@ -240,6 +263,55 @@ pub fn run(storage: &dyn ProtocolStateStorage) -> ConformanceReport {
report
}
+/// The isolation check: the same id under two key types, and an id under
+/// two others that must not appear in the first one's listing.
+///
+/// Only a collision fails it. A value that comes back wrong in another way
+/// is the round-trip checks' to report, and a listing that fails is the
+/// listing checks'; neither is evidence that a listed delete would reach
+/// outside the probe key types. The probe records are removed with point
+/// deletes whatever the outcome.
+fn key_types_are_isolated(storage: &dyn ProtocolStateStorage) -> std::result::Result<(), String> {
+ let outcome = (|| -> std::result::Result<(), String> {
+ storage
+ .store(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_PROBE_ID, b"theirs")
+ .and_then(|()| storage.store(CONFORMANCE_KEY_TYPE, ISOLATION_PROBE_ID, b"mine"))
+ .and_then(|()| {
+ storage.store(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_LISTING_PROBE_ID, b"x")
+ })
+ .and_then(|()| {
+ storage.store(CONFORMANCE_KEY_TYPE_SUFFIXED, ISOLATION_LISTING_PROBE_ID, b"x")
+ })
+ .map_err(|err| {
+ format!("store failed, so isolation could not be checked and nothing was deleted: {err}")
+ })?;
+ if let Ok(Some(other)) = storage.load(CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_PROBE_ID) {
+ if other == b"mine" {
+ return Err(
+ "the same key id in two key types collided: a write under one \
+ replaced the other"
+ .to_string(),
+ );
+ }
+ }
+ if let Ok(listed) = storage.list_keys(CONFORMANCE_KEY_TYPE) {
+ if listed.iter().any(|key| key == ISOLATION_LISTING_PROBE_ID) {
+ return Err("list_keys named a record written under another key type".to_string());
+ }
+ }
+ Ok(())
+ })();
+ for (key_type, key_id) in [
+ (CONFORMANCE_KEY_TYPE, ISOLATION_PROBE_ID),
+ (CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_PROBE_ID),
+ (CONFORMANCE_KEY_TYPE_OTHER, ISOLATION_LISTING_PROBE_ID),
+ (CONFORMANCE_KEY_TYPE_SUFFIXED, ISOLATION_LISTING_PROBE_ID),
+ ] {
+ let _ = storage.delete(key_type, key_id);
+ }
+ outcome
+}
+
/// Remove every record the suite writes, in both key types.
fn cleanup(storage: &dyn ProtocolStateStorage) {
for key_type in [CONFORMANCE_KEY_TYPE, CONFORMANCE_KEY_TYPE_OTHER] {
diff --git a/crates/offline-protocol/tests/file_store_relative_root.rs b/crates/offline-protocol/tests/file_store_relative_root.rs
new file mode 100644
index 000000000..aeb3d30c4
--- /dev/null
+++ b/crates/offline-protocol/tests/file_store_relative_root.rs
@@ -0,0 +1,45 @@
+//! The built-in file stores open over a relative root on the first try.
+//!
+//! A store creates its directories and flushes each one's parent so the new
+//! entry survives a crash. For a one-component relative root such as `keys`,
+//! `Path::parent` is the empty path, and flushing that fails with "not
+//! found": the first open failed and the second, finding the directory
+//! there, succeeded. Every unit test opens an absolute temporary root, so
+//! none of them reaches it.
+//!
+//! This is its own test binary because it changes the process's working
+//! directory, which every other test in a shared binary would see.
+
+#![cfg(feature = "file-store")]
+
+use offline_protocol::file_store::{account_storage_namespace, FileStorePair, StaticStoreKey};
+
+#[test]
+fn a_pair_opens_over_one_component_relative_roots_on_the_first_try() {
+ let base = std::env::temp_dir().join(format!(
+ "offline-protocol-relative-root-{}-{}",
+ std::process::id(),
+ std::time::SystemTime::now()
+ .duration_since(std::time::UNIX_EPOCH)
+ .map(|elapsed| elapsed.as_nanos())
+ .unwrap_or(0)
+ ));
+ std::fs::create_dir_all(&base).expect("create the working directory");
+ let previous = std::env::current_dir().expect("current directory");
+ std::env::set_current_dir(&base).expect("enter the working directory");
+
+ let namespace = account_storage_namespace("com.example.relative", "alice");
+ let outcome = FileStorePair::open("keys", "state", &namespace, &StaticStoreKey::new([7; 32]));
+ let created = (base.join("keys").is_dir(), base.join("state").is_dir());
+
+ std::env::set_current_dir(&previous).expect("leave the working directory");
+ let pair = outcome.expect("the first open over relative roots");
+ pair.close();
+ let _ = std::fs::remove_dir_all(&base);
+
+ assert_eq!(
+ created,
+ (true, true),
+ "both roots are made under the working directory"
+ );
+}
diff --git a/docs/bridges/README.md b/docs/bridges/README.md
index c0518f994..da19a73c5 100644
--- a/docs/bridges/README.md
+++ b/docs/bridges/README.md
@@ -411,10 +411,13 @@ binary and empty values, overwrite semantics, delete idempotence, key-type
isolation, listing accuracy, composed and long key ids, large records, and
delete completeness. Each check exists because that defect is invisible
until data is missing. The suite writes only under its own probe key types
-and deletes everything it wrote, which keeps a live store intact only if the
-backend keeps key types apart: on one that merges them, listing a probe key
-type names real records too, and the suite's cleanup deletes what it lists.
-Probe a new backend on a scratch instance until it is green.
+and deletes everything it wrote. That keeps a live store intact only if the
+backend keeps key types apart, since on one that merges them, listing a
+probe key type names real records too. So isolation is checked first, with
+point writes and point deletes of ids nothing else uses, including a probe
+type that ends like another (the SDK's own key types are related that way),
+and a failure ends the run before anything is listed and deleted. Probe a
+new backend on a scratch instance until it is green all the same.
Green on either suite is not a persistence test. Every check runs against one
open instance, so a backend that forgets everything on reopen, or that
diff --git a/docs/bridges/swift.md b/docs/bridges/swift.md
index 8bbc4378c..3726199b9 100644
--- a/docs/bridges/swift.md
+++ b/docs/bridges/swift.md
@@ -259,17 +259,22 @@ Three things hold the invariant:
- `bindings/react-native/scripts/build-uniffi-ios.sh` reads the target out of
the podspec and exports it before it builds. The number is written once, in
the podspec. The SDK's own Rust code is compiled for it too.
-- After the build, the script reads every object in every slice and refuses to
- package one that is too new. An archive it could not read fails the same
- way: one with no stamp in it, and one `otool` read part of before giving
- up.
+- `package_xcframework` reads every object in every slice before it packages
+ anything, and refuses an archive that holds one too new. An archive it
+ could not read fails the same way: one with no stamp in it, and one
+ `otool` read part of before giving up. The check is inside the packaging
+ function, not beside the call, so the pod's XCFramework cannot be packaged
+ without it. The Swift package's XCFramework is built by
+ `scripts/package-swiftpm-xcframework.sh`, which does not check; the
+ `Swift Package` job runs the check on its archive as a step of its own.
- The gate needs `otool`, so in CI it runs where the library is built on a
Mac: in a release, on all three archives, and in the `Swift Package` job,
on the simulator archive of every pull request.
`scripts/tests/test-ios-min-os.sh`, at the repository root, runs on every
pull request on Linux. It covers what can go wrong without `otool`: the
- parser, the podspec reader, a failing `otool`, and that the build script
- still calls the gate on all three archives.
+ parser, the podspec reader, a failing `otool`, and the packaging function
+ itself, driven with stand-ins for `otool`, `lipo` and `xcodebuild`: each
+ archive is held to its own ceiling, and a refused one packages nothing.
The podspec has one reader, `ios_deployment_target` in
`bindings/react-native/scripts/shared/xcframework.sh`.
@@ -281,8 +286,10 @@ the release would have built for a number the package refused.
A target directory built for one deployment target keeps its Rust objects
when the target changes, because cargo does not rebuild a crate for a new
value of the variable. Only the C compiled through `cc-rs` is redone. A
-lowered target therefore fails the gate on Rust objects until `cargo clean`,
-locally or in a CI cache.
+lowered target therefore fails the gate on Rust objects until `cargo clean`.
+Both CI caches that hold iOS objects, the `Swift Package` job's and the
+release's, add the podspec's hash to their key, so a changed target starts
+from a cold cache there.
## Testing
diff --git a/docs/mls-integration.md b/docs/mls-integration.md
index 1b2ca0053..ae8c5241f 100644
--- a/docs/mls-integration.md
+++ b/docs/mls-integration.md
@@ -428,9 +428,13 @@ and binds the two so a later open knows they belong together (see
a store sealed under another identity's record key. `FileStorePair`
records which MLS store a state store belongs with: its first open writes
one random pairing id into both (`mls-store.pair`, sealed under the store
- key, and `state-store.pair`, in the clear), the MLS store first, and a
- later open compares them. Neither file is a record, and a binding's
- provider never reads them.
+ key, and `state-store.pair`, in the clear), the state store first, so a
+ crash between the two writes leaves the MLS store with no id rather than
+ two stores on different ones. A later open compares them. Where they
+ differ, a sealed record that opens under this MLS store's record key
+ still admits the state root, since only this identity's records open under
+ it; nothing less does. Neither file is a record, and a binding's provider
+ never reads them.
- **The records alone cannot settle ownership.**
`FileProtocolStateStorage::sealed_state` asks the sealed records whether
they open under the MLS store's record key, without changing either
diff --git a/scripts/tests/test-ios-min-os.sh b/scripts/tests/test-ios-min-os.sh
index ba338ba55..eb41ba60b 100755
--- a/scripts/tests/test-ios-min-os.sh
+++ b/scripts/tests/test-ios-min-os.sh
@@ -22,7 +22,7 @@ source "$RN_SCRIPTS/shared/xcframework.sh"
FAILURES=0
ASSERTIONS=0
# A test that stops asserting passes. Raise this with every check added.
-EXPECTED_ASSERTIONS=59
+EXPECTED_ASSERTIONS=69
WORK="$(mktemp -d)"
trap 'rm -rf "$WORK"' EXIT
@@ -232,17 +232,88 @@ gate "an archive with no stamp" "not otool output" 0 13.0 1 "found no minimum OS
# up. The ones it never reached are the ones nothing looked at.
gate "an archive otool read part of" "$GOOD" 1 13.0 1 "otool could not read all of"
+echo "the packaging"
+
+# The gate lives inside package_xcframework, so the release cannot package an
+# archive it did not check. These drive the function itself, with stand-ins
+# for otool (a fixture per archive), lipo and xcodebuild (which log their
+# calls), so a gate that is dropped, skipped or given the wrong ceiling fails
+# here whatever the build script around it looks like.
+cat >"$STUBS/otool" <<'STUB'
+#!/usr/bin/env bash
+cat "$2.otool"
+STUB
+cat >"$STUBS/lipo" <<'STUB'
+#!/usr/bin/env bash
+echo "lipo $*" >>"$PACKAGING_LOG"
+while [ "$#" -gt 0 ]; do
+ if [ "$1" = "-output" ]; then : >"$2"; fi
+ shift
+done
+STUB
+cat >"$STUBS/xcodebuild" <<'STUB'
+#!/usr/bin/env bash
+echo "xcodebuild $*" >>"$PACKAGING_LOG"
+STUB
+chmod +x "$STUBS/otool" "$STUBS/lipo" "$STUBS/xcodebuild"
+
+# archive : an archive on disk and the otool dump the stand-in
+# prints for it.
+archive() {
+ : >"$WORK/$1"
+ object "$1.o" LC_BUILD_VERSION minos "$2" >"$WORK/$1.otool"
+}
+
+# package
+# . The function runs in a subshell because it sets and clears
+# an EXIT trap of its own, which would otherwise replace this script's.
+package() {
+ local name="$1" status=0
+ archive device.a "$2"
+ archive sim_arm64.a "$3"
+ archive sim_x86_64.a "$4"
+ export PACKAGING_LOG="$WORK/packaging.log"
+ : >"$PACKAGING_LOG"
+ rm -rf "$WORK/out"
+ mkdir -p "$WORK/out"
+ (
+ PATH="$STUBS:$PATH"
+ package_xcframework "$WORK/out" "$WORK/device.a" "$WORK/sim_arm64.a" \
+ "$WORK/sim_x86_64.a" "$5"
+ ) >"$WORK/package.out" 2>&1 || status=$?
+ equal "$name: exit status" "$status" "$6"
+ if [ "$6" != 0 ]; then
+ equal "$name: nothing is packaged" "$(wc -l <"$PACKAGING_LOG" | tr -d ' ')" "0"
+ fi
+}
+
+package "every archive at the target" 13.0 14.0 13.0 13.0 0
+equal "one simulator archive is made" "$(grep -c '^lipo -create' "$PACKAGING_LOG")" "1"
+equal "one XCFramework is made" "$(grep -c '^xcodebuild -create-xcframework' "$PACKAGING_LOG")" "1"
+equal "of two slices" "$(grep -o -- '-library' "$PACKAGING_LOG" | wc -l | tr -d ' ')" "2"
+
+package "a device archive built for a newer iOS" 26.5 14.0 13.0 13.0 1
+# The arm64 simulator slice is held to 14.0, never higher.
+package "an arm64 simulator archive above its floor" 13.0 14.5 13.0 13.0 1
+# The floor is the arm64 simulator's alone: the Intel slice keeps the target.
+package "an Intel simulator archive at the arm64 floor" 13.0 14.0 14.0 13.0 1
+package "no deployment target" 13.0 14.0 13.0 "" 1
+if grep -qF "needs the deployment target" "$WORK/package.out"; then
+ pass "no deployment target: says it needs one"
+else
+ fail "no deployment target: does not say it needs one: $(tr '\n' ' ' <"$WORK/package.out")"
+fi
+
echo "the wiring"
-# The gate runs only in a release, so nothing else notices it being removed
-# from the script that calls it.
+# The build script reads the target, exports it before building, and hands it
+# to the packaging, which checks against it.
BUILD="$RN_SCRIPTS/build-uniffi-ios.sh"
line_of() { grep -nF -- "$1" "$BUILD" | head -1 | cut -d: -f1; }
EXPORT_LINE="$(line_of 'export IPHONEOS_DEPLOYMENT_TARGET="$IOS_DEPLOYMENT_TARGET"')"
READ_LINE="$(line_of 'IOS_DEPLOYMENT_TARGET="$(ios_deployment_target ')"
BUILD_LINE="$(line_of 'cargo build --release --target "$arch"')"
-PACKAGE_LINE="$(line_of 'package_xcframework \')"
if [ -n "$READ_LINE" ] && [ -n "$EXPORT_LINE" ] && [ "$READ_LINE" -lt "$EXPORT_LINE" ]; then
pass "the target is read from the podspec, then exported"
@@ -256,21 +327,11 @@ else
fail "it is not exported before the build"
fi
-for arch in aarch64-apple-ios aarch64-apple-ios-sim x86_64-apple-ios; do
- GATE_LINE="$(line_of "assert_archive_min_os \"\$(static_lib_for $arch)\"")"
- if [ -n "$GATE_LINE" ] && [ -n "$PACKAGE_LINE" ] &&
- [ "$BUILD_LINE" -lt "$GATE_LINE" ] && [ "$GATE_LINE" -lt "$PACKAGE_LINE" ]; then
- pass "$arch is checked after it is built and before it is packaged"
- else
- fail "$arch is not checked between the build and the packaging"
- fi
-done
-
-if grep -A1 -F 'assert_archive_min_os "$(static_lib_for aarch64-apple-ios-sim)"' "$BUILD" |
- grep -qF 'arm64_simulator_ceiling "$IOS_DEPLOYMENT_TARGET"'; then
- pass "the simulator slice is held to a ceiling worked out from the target"
+if awk '/^package_xcframework /{p=1} p{print} p&&!/\\$/{exit}' "$BUILD" |
+ grep -qF '"$IOS_DEPLOYMENT_TARGET"'; then
+ pass "the packaging is handed the target it checks against"
else
- fail "the simulator slice is held to a ceiling that is written down"
+ fail "the packaging is not handed the target"
fi
if [ "$ASSERTIONS" != "$EXPECTED_ASSERTIONS" ]; then