feat: make Value Send by switching Text/Blob payloads to Arc (#688)

.openspec/adr/0039-value-payloads-are-arc-not-rc.md

@@ -0,0 +1,84 @@
+# 0039: `Value`'s text and blob payloads are `Arc`, not `Rc`
+
+Date: 2026-09-04
+
+## Context
+
+`Value::Text(Rc<str>)` and `Value::Blob(Rc<[u8]>)` made `Value` itself
+`!Send`. A result row is a `Vec<Value>`, so no query result could cross a
+thread boundary at all.
+
+That blocks spec 013's embedding API outright. Its Requirement 4 asks for a
+`Send + Sync` connection handle, and ADR-0034 (as filed in PR #678) proposes a
+worker thread that "creates its `Rc` graph on a thread it owns and never lets
+it leave". Rows are precisely the thing that must leave, so the proposed design
+is not sufficient as written — a gap neither the spec nor that ADR records,
+because both locate the `Send` problem in the pager alone.
+
+**The obvious objection is that ADR-0013 and ADR-0017 already rejected `Arc`.
+They did not — not for this.** Both are about the *pager*: one `Vm` shares a
+page source across N cursors via cheap `Rc` clones, and making that `Arc` would
+tax the Tier 0 read path to serve a requirement that lives at the connection
+boundary. `Value`'s payloads were never their subject. The natural reading of
+those two ADRs is nevertheless "`Arc` anywhere is settled against", which is
+why this needs writing down rather than being left as an apparent
+contradiction.
+
+Spike 014 (#682) measured the change instead of arguing it:
+
+- **+22/−17 across 6 files.** Nearly every construction site writes
+  `Value::Text(s.to_string().into())`, and `.into()` is identical for `Rc<str>`
+  and `Arc<str>`, so only the ~12 sites that *name* the type needed editing.
+- **1562 tests pass, 0 fail** — identical to the `Rc` baseline.
+- **No measurable read-path cost.** `full_drain/batch` is single-threaded with
+  no thread boundary, so it isolates the tax: `Rc` runs spanned 5.42–5.62 ms,
+  `Arc` runs 5.24–5.33 ms. One comparison reported `Arc` 5.6% *faster*, which
+  is not a credible speedup from adding atomics — the honest reading is that
+  the difference sits inside run-to-run variance.
+- It removes the boundary copy permanently: handing rows over untouched ran
+  5.13 ms against 7.34 ms for the owned-copy alternative.
+
+## Decision
+
+`Value::Text` and `Value::Blob` hold `Arc<str>` and `Arc<[u8]>`. `Value` is
+therefore `Send + Sync`, enforced by a `const` assertion in
+`src/record/value.rs` rather than by convention.
+
+`Rc<dyn PageSource>` and `Rc<RefCell<Pager>>` are **unchanged**. ADR-0013 and
+ADR-0017 remain in force on exactly the question they decided.
+
+## Alternatives rejected
+
+- **An owned copy at the API boundary** (`String`/`Vec<u8>`), leaving `Value`
+  as `Rc`. Works, and was the spike's first prototype — but it costs an
+  allocation and copy per text and blob value on every row, forever, and it
+  puts two value types in front of consumers: the engine's and the API's.
+  Measured 7.34 ms against 5.13 ms on a 50,000-row drain. Kept as the fallback
+  if the `Arc` change is ever judged too invasive on principle, since the
+  measured case against it is the only case against it.
+- **Handing back encoded record bytes** and making the consumer decode. No
+  per-value copies, but it moves the record format into the public API and
+  pushes decoding onto every consumer.
+- **`Arc` for the pager too**, unifying the story. Rejected: that is the change
+  ADR-0013 and ADR-0017 actually considered and refused, and nothing here
+  disturbs their reasoning. The read path shares a page source across cursors
+  on one thread; the connection boundary is a different problem with a
+  different answer.
+
+## Consequences
+
+- `Value` is `Send + Sync` and consumers may move rows between threads. The
+  embedding API's worker-thread design (spec 013/Req 4) becomes implementable
+  without a second value type.
+- Cloning a `Value` now costs an atomic increment rather than a non-atomic one.
+  Measured as unobservable on the read path, but the spike's noise floor is a
+  few percent, so **this should be re-measured on `tests/performance/engine.rs`
+  against the pinned oracle before the figure is quoted anywhere as settled.**
+- ADR-0034 (PR #678) needs amending or superseding on its Req 4 rationale: the
+  `Send` obstacle is not only the pager. That is #678's to fix, not this ADR's.
+- Anything that reconstructs a `Value` from a shared buffer now needs `Arc`
+  semantics; `src/record/decode.rs` and the sorter/hash-aggregation paths were
+  the only such sites and are updated here.
+- ADR numbering is contended across three unmerged branches: `feat/683` claims
+  0038, this claims 0039, and PR #678's claims whatever is free at its merge.
+  Numbers are only settled by merge order, so the last to land renumbers.

.openspec/adr/index.md

@@ -42,3 +42,4 @@ Specs record what the system must do; ADRs record **why it is shaped this way**
 | [0036](0036-pragma-synchronous-fsync-policy.md) | `PRAGMA synchronous` fsync-skip policy, and why `SynchronousMode` lives in `header.rs` | 2026-08-29 |
 | [0037](0037-macos-plain-fsync-not-fullfsync.md) | On macOS, `Vfs::sync` calls plain `fsync(2)`, not `std`'s `F_FULLFSYNC` | 2026-08-30 |
 | [0038](0038-cargo-registry-opt-in-not-committed.md) | Artifactory Cargo access is opt-in local config, never a committed source replacement | 2026-09-03 |
+| [0039](0039-value-payloads-are-arc-not-rc.md) | `Value`'s text and blob payloads are `Arc`, not `Rc` | 2026-09-04 |

src/record/decode.rs

@@ -1,6 +1,6 @@
 // Copyright 2026 Schuberg Philis
 // SPDX-License-Identifier: Apache-2.0
-use std::rc::Rc;
+use std::sync::Arc;
 
 use super::error::RecordError;
 use super::value::{TextEncoding, Value};
@@ -389,17 +389,17 @@ pub(crate) fn decode_serial_value(
     }
 }
 
-/// Decodes text bytes straight into `Rc<str>`. The UTF-8 case (by far the
-/// common one) builds the `Rc<str>` directly from the validated byte slice
+/// Decodes text bytes straight into `Arc<str>`. The UTF-8 case (by far the
+/// common one) builds the `Arc<str>` directly from the validated byte slice
 /// instead of routing through an intermediate `String`, avoiding a second
 /// allocation and copy per text column.
-fn decode_text(bytes: &[u8], encoding: TextEncoding) -> Result<Rc<str>, RecordError> {
+fn decode_text(bytes: &[u8], encoding: TextEncoding) -> Result<Arc<str>, RecordError> {
     match encoding {
         TextEncoding::Utf8 => std::str::from_utf8(bytes)
-            .map(Rc::from)
+            .map(Arc::from)
             .map_err(|_| RecordError::InvalidUtf8),
-        TextEncoding::Utf16Le => decode_utf16(bytes, u16::from_le_bytes).map(Rc::from),
-        TextEncoding::Utf16Be => decode_utf16(bytes, u16::from_be_bytes).map(Rc::from),
+        TextEncoding::Utf16Le => decode_utf16(bytes, u16::from_le_bytes).map(Arc::from),
+        TextEncoding::Utf16Be => decode_utf16(bytes, u16::from_be_bytes).map(Arc::from),
     }
 }
 

src/record/value.rs

@@ -1,6 +1,6 @@
 // Copyright 2026 Schuberg Philis
 // SPDX-License-Identifier: Apache-2.0
-use std::rc::Rc;
+use std::sync::Arc;
 
 /// A single decoded column value, per SQLite's dynamic type system.
 #[derive(Debug, Clone, PartialEq)]
@@ -12,11 +12,23 @@ pub enum Value {
     /// An 8-byte IEEE 754 floating-point value.
     Real(f64),
     /// A text value, decoded according to the database's `TextEncoding`.
-    Text(Rc<str>),
+    Text(Arc<str>),
     /// An uninterpreted byte sequence.
-    Blob(Rc<[u8]>),
+    Blob(Arc<[u8]>),
 }
 
+/// `Value` must stay `Send + Sync` (#688): the embedding API's
+/// connection handle hands result rows to another thread, and a row is
+/// a `Vec<Value>`. `Rc` payloads made that impossible, and nothing but a
+/// compile-time check keeps it from silently regressing — swapping
+/// either payload back to `Rc` would otherwise only fail much later, in
+/// whichever consumer tried to cross a thread.
+///
+/// This is deliberately *not* a claim about `Pager`/`PageSource`, which
+/// stay `Rc` per ADR-0013 and ADR-0017. See ADR-0039.
+const fn assert_value_send_sync<T: Send + Sync>() {}
+const _: () = assert_value_send_sync::<Value>();
+
 /// The database's text encoding, from database header byte 56.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum TextEncoding {

src/vdbe/cursor.rs

@@ -51,6 +51,7 @@
 use std::cell::RefCell;
 use std::collections::{BTreeMap, HashMap};
 use std::rc::Rc;
+use std::sync::Arc;
 
 use crate::btree::{self, IndexCursor, TableCursor};
 use crate::record::{
@@ -115,7 +116,7 @@ pub(crate) enum CursorSlot {
         /// for table cursors. Valid only while `cached_blob` still
         /// points at the same allocation as `register`'s current value.
         header_cache: RowHeaderCache,
-        cached_blob: Option<Rc<[u8]>>,
+        cached_blob: Option<Arc<[u8]>>,
     },
     Sorter(crate::vdbe::sorter::SorterState),
     /// A hash-aggregation table (#570) — opened by `HashAggOpen`, the
@@ -586,9 +587,9 @@ fn normalize_key_values(values: &[Value], collations: &[Collation]) -> Vec<Value
         .zip(collations.iter())
         .map(|(v, collation)| match (v, collation) {
             (Value::Text(s), Collation::NoCase) => {
-                Value::Text(Rc::from(s.to_ascii_lowercase().as_str()))
+                Value::Text(Arc::from(s.to_ascii_lowercase().as_str()))
             }
-            (Value::Text(s), Collation::RTrim) => Value::Text(Rc::from(s.trim_end_matches(' '))),
+            (Value::Text(s), Collation::RTrim) => Value::Text(Arc::from(s.trim_end_matches(' '))),
             _ => v.clone(),
         })
         .collect()
@@ -960,7 +961,7 @@ fn read_row_column(
                 cached_blob,
                 ..
             } => {
-                if !cached_blob.as_ref().is_some_and(|b| Rc::ptr_eq(b, &bytes)) {
+                if !cached_blob.as_ref().is_some_and(|b| Arc::ptr_eq(b, &bytes)) {
                     header_cache.invalidate();
                     *cached_blob = Some(bytes.clone());
                 }
@@ -4139,7 +4140,7 @@ mod tests {
         // ever produce on its own.
         state.set_current(Some(btree::IndexRow {
             payload: btree::Payload::Owned(encode_record(
-                &[Value::Text(Rc::from("not-a-rowid"))],
+                &[Value::Text(Arc::from("not-a-rowid"))],
                 TextEncoding::Utf8,
             )),
         }));

src/vdbe/hash_agg.rs

@@ -57,7 +57,7 @@
 //!    insert), so no query can reach it.
 
 use std::collections::HashMap;
-use std::rc::Rc;
+use std::sync::Arc;
 
 use crate::record::{Collation, Value};
 use crate::vdbe::affinity::{apply_affinity, Affinity};
@@ -80,7 +80,7 @@ struct GroupSlot {
     /// observably pick the group's first row, and the sort strategy —
     /// whose sort is stable, so a group's ties stay in scan order —
     /// picks the same one.
-    row: Rc<[u8]>,
+    row: Arc<[u8]>,
     /// This group's key values in key order (not record order),
     /// post-affinity, used only to order the groups at `HashAggRewind`.
     key_values: Vec<Value>,

src/vdbe/result.rs

@@ -5,7 +5,7 @@
 //! record serialization (`MakeRecord`, reusing spec 003's on-disk record
 //! encoding byte-for-byte), and row emission (`ResultRow`).
 
-use std::rc::Rc;
+use std::sync::Arc;
 
 use crate::record::{encode_record_into, TextEncoding, Value};
 use crate::vdbe::affinity::{apply_affinity, Affinity};
@@ -166,7 +166,7 @@ pub fn make_record(vm: &mut Vm, instr: &Instruction) -> Result<Step, ExecError>
         &mut scratch,
         &mut encode_scratch,
     );
-    let payload: Rc<[u8]> = Rc::from(scratch.as_slice());
+    let payload: Arc<[u8]> = Arc::from(scratch.as_slice());
     *vm.record_scratch() = scratch;
     *vm.encode_scratch() = encode_scratch;
     *vm.make_record_values_scratch() = values;

src/vdbe/sorter.rs

@@ -286,7 +286,7 @@ pub fn sorter_insert(vm: &mut Vm, instr: &Instruction) -> Result<Step, ExecError
     Ok(Step::Next)
 }
 
-type SorterRow = (std::rc::Rc<[u8]>, Vec<Value>);
+type SorterRow = (std::sync::Arc<[u8]>, Vec<Value>);
 
 /// Restores the max-heap property (root = worst row, per `compare_rows`)
 /// after appending a new element at `buf`'s end — bubbles it up while