ci: detect memory misuse before merge, and fix the five lifetime bugs it surfaces

[ihrfv]

Sits on top of #4 (the vendored-submodule build), now merged — the diff is this
branch's own 12 commits.

Why

CI ran fmt, clippy, and cargo test against a release build of the C
engine. Nothing in that pipeline can see a use-after-free, so five lifetime
defects — all reachable from safe code, one of them a segfault from ten lines —
sat in the tree unnoticed.

So this branch adds the detector first, then fixes what it reports: the four CI
commits lead, and the fixes that follow are the bugs the new leg finds.

The detector

Miri cannot run this crate at all. It interprets MIR and cannot execute compiled
foreign code, while every entry point here crosses into librayforce.a —
Runtime::scope calls ray_runtime_create, Value::drop calls ray_release.
cargo miri test dies on the first one. There is no pure-Rust subset worth
interpreting.

AddressSanitizer is no better, and the engine's own source says so. The heap is
mmap-backed — ray_sys_alloc (src/mem/sys.c) goes straight to ray_vm_alloc
and never calls malloc, which is exactly what ASan, LeakSanitizer and Valgrind
track. From src/mem/heap.c:

ASan cannot see use-after-free inside the pool allocator, so this is the tool
of choice for chasing double releases.

"This" is DFD, a stale-pointer detector already in the core: a shadow set of
block addresses currently on a freelist, consulted by ray_retain and
ray_release (src/mem/cow.c), aborting with a backtrace on a hit. It is gated
on the core being compiled with -DDEBUG and on RAY_DFD=1 at runtime.
build.rs ran make lib, which archives release objects — so the detector built
for this exact bug class had never been compiled into anything this crate links.

RAYFORCE_CORE_DEBUG=1 now builds that flavour. The flavour joins RAY_VERSION
and GIT_HASH in the stamp invalidate_on_stamp_change already compares, since
both flavours compile to the same object names and make tracks header
dependencies but not flag changes — without it, a flavour switch would archive a
mixed library.

Two CI gaps closed

The debug leg. A core: [release, debug] matrix axis. fmt and clippy are
flavour-independent and stay on the release leg. The check names become
test (release) / test (debug) — branch protection needs updating to require
both.

tests/ipc.rs was not running. It spawns a real server to exercise
TcpClient and returned early without one — and an early return reports as a
pass, so the absence of coverage was indistinguishable from the coverage
passing. That is the path two of the fixes below land on, so they would have
shipped with nothing executed. CI now builds the server binary, and
RAYFORCE_REQUIRE_SERVER=1 turns a missing one into a failure rather than a
skip. The binary is built in the staged OUT_DIR tree that build.rs just
compiled, so it costs one main.c compile plus a link — measured at 0.23s, not a
second engine build.

tests/q_real.rs still opts out via RAYFORCE_Q_ADDR. It needs a real q
server, which cannot be provisioned on a runner. That skip is genuine and is now
the only one.

The root cause the bugs share

Two things are shared across the FFI boundary, and only one was counted.

ray_t.rc counts references to an object, and the crate tracked it correctly:
Clone retains, Drop releases. Nothing counted references to the heap those
objects live in — and that is the one that matters, because
ray_runtime_destroy never consults rc. It munmaps every pool outright
(ray_heap_destroy, src/mem/heap.c). An object with rc == 5 is unmapped
exactly like one with rc == 1.

So a handle outliving its runtime does not point at freed bytes that happen to
still be readable. It points at unmapped address space, and there is no check
at the point of use that can make dereferencing it safe: by then the thing to
check is the pointer, and the pointer is what became invalid.

Rather than add the missing count, this branch removes the shape.
Runtime::new becomes private and Runtime::scope(|rt| { … }) is the only way
to a runtime. It creates the runtime, hands the closure a &Runtime that cannot
be dropped or moved out of, and tears it down when the closure returns — on the
error path and on unwind alike. The closure's locals go first, so every Value
is dropped before the heap it points into is unmapped.

What keeps values inside the scope is the !Send marker the crate already
carried, read as a bound. scope requires Send of its return type and of the
closure, and Value, Table, Fn, TcpClient and QConnection are all
!Send, so returning one — or assigning one into a variable declared outside —
is a compile error reading required by a bound in Runtime::scope.

The consequences are worth stating plainly:

• Value carries no bookkeeping. It stays one pointer wide, with nothing
  added to clone or drop. A heap-level refcount would have cost a relaxed atomic
  on every construct, clone and drop; bounding the runtime's life costs nothing
  at runtime at all.
• The diagnostic talks about threads when no thread is involved. An unrelated
  !Send capture — an Rc, a RefCell borrow — is refused too. Construct such
  values inside the closure, or move them in.
• One escape slips through. A closure that stashes into a thread_local!
  captures nothing, so it satisfies the bound. That is documented on
  Runtime::scope rather than papered over.
• A nested scope errors instead of starting a second runtime; the core
  permits one heap at a time.

The bugs

A Value outliving its Runtime segfaults. The case above, and the one the
debug leg actually reports. It surfaced at process exit, far from its cause.

The connection types close into an unmapped heap. TcpClient and
QConnection had no liveness tracking of any kind, and their Drops call
ray_ipc_close / q_close, both of which reach into the runtime. Both are now
!Send/!Sync like every other handle, which is what the scope's bounds read,
and both Drops run before the runtime's.

Building a value did not require a runtime. Value::i64(1) with no runtime
was safe Rust calling straight into the engine with no check at all. Nothing
crashed, which is why it went unnoticed: ray_alloc lazily maps a heap when none
exists, so the value landed in an orphan one. The sharp case is symbols, which
are runtime-scoped — Value::sym("hello") returned an empty symbol, dropping
the string with no error anywhere. Verified before the fix: it formatted as '.

The runtime leaks its event loop. TcpClient::connect installs a poll on
first use and ray_runtime_destroy never touches it. Teardown now takes it down
before the heap goes — closing a selector releases engine objects held for it,
so that order is load-bearing. Found by reading, not by the debug leg: this is a
leak, and no leak checker tracks mmap. What CI adds here is that the path now
executes at all.

QConnection was Send and Sync. A bare file descriptor with no
PhantomData, so it inferred both, while Value, Runtime and TcpClient are
neither. execute interns symbols and builds engine objects belonging to the
runtime's thread, and Sync additionally allowed two threads to interleave
writes on one socket. Compile-time properties need compile-time tests, so all
four handle-carrying types now ship compile_fail doctests — each paired with a
control identical but for the bound, since a compile_fail block passes whenever
the snippet fails to build for any reason.

Building with --no-default-features (no chrono) is warning-free again.

The rule this settles on

One predicate, not two: a live Runtime guard is required for everything
except reading and dropping handles you already hold. eval, set_global,
get_global, the value constructors and the connection constructors all answer
to the same is_live(), which is true only inside a scope.

Migration

Mechanical — delete let _rt = Runtime::new()?; and wrap the body in
Runtime::scope(|rt| { … Ok(()) })?. Every test, the example, the benchmarks and
65 snippets across 34 docs pages moved that way.

Cost

Value stays 8 bytes with no per-handle bookkeeping. No API changes shape apart
from Runtime::new becoming Runtime::scope. The debug leg builds the engine at
-O0, which is fast — the release -O3 -march=native build is the slow one.

Verification

Run locally against the vendored core this branch now sits on (v2.5.8,
f0d4bb4):

• The detector is armed, proven by positive control. A deliberate double
  release through the sys escape hatch aborts with
  === DFD: ray_release (stale release of freed block) on FREED block 0x... ===
  and a backtrace through ray_release. The debug archive contains
  ray_dfd_check_live; the release archive does not (nm reports 2 symbols vs
  0). A clean run therefore means something.
• Both legs green at HEAD, 109 tests each (87 integration + 22 doctests),
  zero IPC skips, fmt and clippy -D warnings clean, --no-default-features
  warning-free — locally, and on this PR's own CI run, where test (release) and
  test (debug) both pass and the four server-backed IPC tests report ok on
  each leg.
• tests/ipc.rs runs against a real spawned server, including a client that
  is closed before its scope ends and a failed connect. The server binary carries
  DFD on the
  debug leg and reports 2.5.8 (f0d4bb4) — the pinned core, not this
  repository's HEAD, which is what an unset RAY_VERSION would have stamped from
  inside OUT_DIR.
• Every commit builds, checked one by one, so the history bisects.
• The empty-symbol bug was reproduced before being fixed, and tests/no_runtime.rs
  pins it in its own test binary — the property is about a process where no
  runtime was ever created, which cannot be arranged in a file that also builds one.

🤖 Generated with Claude Code

[ihrfv]

Branch protection: the check names changed

This PR splits the test job across a core: [release, debug] matrix, so the single
test check becomes two — test (release) and test (debug). Both are reporting on
this PR already and both pass (run
33077685036:
release 2m44s, debug 1m40s).

The debug leg is the one that carries the point of the branch — it builds the core with
-DDEBUG and runs with RAY_DFD=1, which is the only configuration where the engine's
stale retain/release detector exists at all. If it isn't a required check, a future
use-after-free merges green.

master currently has no protection rule (GET /repos/RayforceDB/rayforce-rs/branches/master
reports "protected": false, and /rulesets is empty), so this would be creating one
rather than editing it. Minimal version, if that's wanted:

gh api -X PUT repos/RayforceDB/rayforce-rs/branches/master/protection --input - <<'JSON'
{
  "required_status_checks": {
    "strict": false,
    "checks": [{"context": "test (release)"}, {"context": "test (debug)"}]
  },
  "enforce_admins": false,
  "required_pull_request_reviews": null,
  "restrictions": null,
  "required_linear_history": false,
  "required_conversation_resolution": true
}
JSON

required_linear_history stays false on purpose — PRs here land as merge commits
(82f2b34), and enabling it would reject them.

Entirely a repo-settings call, not a code one; happy to leave it as-is if you'd rather
keep master unprotected.

[ihrfv]

@protocolstardust Could you please take a look at the PR? thanks :)

[protocolstardust]

Really like the shape of this — bounding the runtime's life instead of adding a heap refcount is the right call, and I checked the poll teardown against the core: ray_runtime_destroy genuinely never touches rt->poll, and ray_poll_destroy really does release engine objects via close_fn, so that ordering is load-bearing exactly like the comment says. Ran both legs locally too, all green, and nm does show the DFD symbols in the debug archive and not in the release one.

A few questions before this goes in.

The liveness flag is process-global, but the thing it guards is thread-local

LIVE is a process-wide AtomicBool, so inside a scope any other thread sees is_live() == true and every guard passes. This segfaults for me:

Runtime::scope(|_rt| {
    std::thread::spawn(|| rayforce::eval("(+ 1 1)")).join().unwrap();
    Ok(())
})

No unsafe anywhere — the spawned closure captures nothing, so it's Send, and the outer one stays Send too. Constructors are quieter but not better: off-thread Value::sym("hello") cheerfully returns "hello" out of an orphan heap nobody will ever unmap.

Any reason not to hang a thread_local! { Cell<bool> } off Runtime::new/Drop and have assert_live read that, keeping LIVE for the "already live" refusal? Asking partly because Runtime::scope's docs say the thread_local! stash is "the single remaining way to build a dangling handle from safe code" — if we're making that claim I'd like it to hold. And if it means changing what the public is_live() means, I'd rather that happened in this major than the next one.

Can the debug leg tell us when it stops detecting?

The whole value of the new axis is that -DDEBUG made it into the archive, and nothing checks that it did. If the RAYFORCE_CORE_DEBUG plumbing ever breaks — env rename, a change in how Actions evaluates that && '1' || '', a build.rs refactor — the leg quietly becomes a second release run and stays green forever. Which is the same shape as the tests/ipc.rs early-return you're fixing here. Would nm ... | grep -q ray_dfd_check_live on the debug leg be worth the line? I confirmed it discriminates cleanly (2 symbols vs 0).

invalidate_on_stamp_change moving out of if stamp_version

Was the effect on RAYFORCE_SRC checkouts intended? As far as I can tell the first cargo build after this now deletes every src/**/*.o and librayforce.a in a directory the user owns, and leaves an untracked .stamp behind. I think it's actually necessary for the flavour switch to be safe — just want to confirm it's deliberate, and if so it probably deserves a changelog line and a tweak to the function's doc comment, which still frames stamping as the OUT_DIR-only case.

[ihrfv]

Thanks — all three land, and the first is a genuine merge blocker. Fixed in four commits on top of b48bd83. Both legs green against the pinned core (v2.5.8, f0d4bb4): 112 tests each — 90 integration + 22 doctests — zero IPC skips, fmt and clippy -D warnings clean, --no-default-features warning-free.

The liveness flag — you're right, and it was worse than a dangling handle

No reason not to; done exactly as you describe. LIVE keeps the "already live" refusal and stays process-wide. A thread_local! { Cell<bool> }, set beside ray_runtime_create and cleared in Drop, is what the assertion reads. The pair cannot drift: Runtime is !Send and never leaves scope, so set and clear always run on one thread.

Two additions to your diagnosis.

Your repro doesn't compile as written. thread::spawn requires T: Send, and Result<Value> isn't — Value carries PhantomData<*mut ()>. Discard the value inside the thread and it compiles and crashes exactly as you said. It is now eval_off_the_runtime_thread_is_refused; on the commit before the fix it takes the whole test binary down with signal: 11, SIGSEGV, in ray_eval_str, which dereferences __VM->nfo with no null check (src/lang/eval.c).

Keeping a second runtime impossible mattered more than it looked. __RUNTIME (src/core/runtime.c:46) is a plain global — a second ray_runtime_create overwrites the first and C says nothing. So the compare-exchange is the only refusal there is, and it stays exactly where it was. a_second_thread_cannot_start_a_runtime pins that, and it passes before the change as well as after; that's what makes it a regression guard rather than a new claim.

On naming: is_live() is now on_runtime_thread(). Splitting the predicates creates a state that did not exist before — live in the process, not on my thread — and is_live() returning false there would have been actively misleading, since it would sit next to a Runtime::scope that refuses. Both refusals now name which bit they are: called off the runtime's thread vs requires a live Runtime, and already live on another thread vs cannot be nested. Breaking, and better in this change than a release later, as you say.

The thread_local!-stash claim is corrected rather than kept — spawning was the easier route and the worse outcome. That section now says what the Send bounds do and don't reach, and that the thread half is a runtime check by necessity: the type system isn't tracking which thread a call happens on.

The debug leg — added, and the one-liner needed a second look

Worth the line. Both legs assert now, in opposite directions: without the release-side control, a grep matching nothing anywhere would pass too — the same pairing every compile_fail doctest here already carries.

grep -c, not grep -q, for a reason I hit while testing it. -q exits on the first match, nm then takes SIGPIPE, and under pipefail the pipeline reports failure — so a present symbol reads as absent. My first debug run printed absent while grep -c on the same archive found 2. Actions' default shell doesn't set pipefail, so it would have worked by luck; under one that does, the check inverts into something that passes for the wrong reason on the release leg. Same shape as the bug it exists to catch. Verified under set -o pipefail against both real archives: 2 on debug, 0 on release.

It proves the detector is compiled in, not that it is armed — that's dfd_enabled() reading RAY_DFD (src/mem/heap.c), and the only proof of arming is the deliberate double release, which aborts the process and so can't live in the suite. Said in the step comment rather than left implied.

invalidate_on_stamp_change — deliberate, and the docs were behind

Intended: 83e029f's message says so — "That check now runs for a RAYFORCE_SRC checkout too, which previously had none". And necessary, as you suspected: core_flavour() is consulted regardless of stamp_version, so a RAYFORCE_SRC tree can flip flavour and would otherwise archive a library mixing release and debug objects, which share every filename.

The claim was stale in more places than the one you found, so it's documented in four:

• invalidate_on_stamp_change — a section saying it runs for both trees, and what it does to a directory the user owns.
• build_core_lib — stamp_version selects RAY_VERSION/GIT_HASH and nothing else; invalidation is deliberately not gated on it.
• docs/.../installation.md claimed a RAYFORCE_SRC checkout is "built in place so your incremental state ... [is] preserved". True except across a flavour switch, which is now called out.
• README, the same note. Plus the CHANGELOG line.

You're right about .stamp: the core's .gitignore covers *.o/*.d/*.a but not it, so it shows up untracked in a core developer's own checkout. Left as a note here rather than opening a one-liner against RayforceDB/rayforce — say the word and I will.

Still outstanding

Branch protection: test (release) and test (debug) both need to be required checks. The gh api call is in the earlier comment; unchanged by any of the above.

[ihrfv]

Follow-up after merge:

bump the version to 1.1.0

.github/workflows/release.yml verifies the tag against the workspace version and fails when they disagree, so the bump has to land as its own commit before tagging. Proposing 1.1.0 in the workspace version key; both crates inherit it.

One thing worth having on the record rather than by accident: tag 1.0.1 shipped pub fn new() and pub fn is_live(), and this branch makes the first private and renames the second to on_runtime_thread(). cargo semver-checks will therefore read the delta as major, and strict semver would say 2.0.0. 1.1.0 is the call here — noting the gap so it's a choice rather than an oversight, and so nobody is surprised by the tooling later.

Migration for anyone on 1.0.x is the two lines already in the changelog: replace let _rt = Runtime::new()?; with Runtime::scope(|rt| { … Ok(()) })?, and is_live() with on_runtime_thread().

Update the merge restriction

instead of a single test check there are now test(debug) and test(release) checks