Fix GC crash from pointer-typed session handles

[George-Payne]

Fixes the fatal invalid pointer found on stack GC crash in the Go bindings (UI-1813, hit twice in CI). The "garbage" values in both crashes were session handles (0x61 = 97, 0x1 = 1): the runtime's handles are small integers cast to gaffer_session*, and s.c() reconstituted the stored uintptr as a fake pointer for every FFI call. Callbacks re-enter Go mid-call, making the goroutine's stack movable while the fake pointer is still live in pointer-mapped slots below - a stack copy then aborts the process. Reproduced near-deterministically pre-fix (-count=100 of the concurrent-GC stress test crashed on its first iteration, twice in a row, with the exact CI signature).

• bindings/go/gaffer.go - Session.c() removed; handles stay uintptr/uintptr_t end-to-end and static C shims (go_session_*, go_debug_*) in the cgo preamble do the (gaffer_session*) casts in C, where the GC can't see them. Also closes the sibling windows: NewSession held the fake pointer live across consumeError, and Destroy/callback registration passed it through Go frames.
• bindings/go/callbacks.go - registration shims (go_on_*) fold both the handle cast and the void* user_data into C; the Go side is keyed by plain uintptr (sessionKey gone, unsafe import gone).
• bindings/go/runtime_test.go - stress-test comment updated to the actual mechanism.
• .changeset/bindings-gc-crash.md - patch: the crash is reachable through the CLI, which links the bindings.

gaffer.h and the runtime are untouched. Post-fix: stress test clean at -count=100, full bindings suite clean with and without -race - the race detector was previously unusable on FFI tests because checkptr fataled on the same illegal conversion; with the conversion gone it runs clean (adding -race to CI is a separate decision, not taken here).

[linear-code]

UI-1813

[qodo-code-review]

PR Summary by Qodo

Fix Go GC crash from pointer-typed session handles

🐞 Bug fix 📝 Documentation 🕐 40+ Minutes

AI Description

• Keep session handles integer-typed in Go; move all casts into C shims.
• Register/invoke callbacks using uintptr keys and C-side user_data shims.
• Document the fix (changeset) and clarify the concurrent-GC stress-test mechanism.

Diagram

graph TD
  A["Go bindings (Session API)"] --> B["C shims (uintptr_t)"] --> C["Gaffer runtime (gaffer.h ABI)"] --> D["Go trampolines (exported)"] --> E["Callback registry (uintptr key)"] --> F["User callbacks"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Change runtime to expose integer-handle API

• ➕ Eliminates the pointer-cast hazard across all language bindings, not just Go
• ➕ Avoids needing per-binding shim layers
• ➖ Requires changing and shipping the runtime/ABI surface; higher blast radius
• ➖ Not feasible if runtime ABI must remain stable

2. Ship a small C wrapper file instead of cgo preamble shims

• ➕ Keeps cgo preamble smaller and centralizes ABI glue in a dedicated C unit
• ➕ Easier to test/validate with a C compiler toolchain independently
• ➖ Adds build-system complexity (extra compiled C sources across platforms)
• ➖ Functionally equivalent to the current approach

Recommendation: The PR’s approach (C shims + uintptr handles in Go) is the best low-risk fix that avoids changing the runtime ABI while fully removing uintptr->pointer reconstitution from Go stack frames (including callback registration paths). The only materially different alternative is changing the runtime API itself, which is a larger cross-language change and out of scope here.

Files changed (4) +166 / -85

Bug fix (2) +152 / -78

callbacks.goRegister callbacks via C shims and key registry by uintptr handle +46/-34

Register callbacks via C shims and key registry by uintptr handle

• Reworks callback registration to avoid passing fake pointer-typed session values through Go frames. Introduces C registration shims (go_on_*) that cast the integer handle to gaffer_session* and store it in user_data in C, while Go-side maps are keyed directly by the uintptr handle.

bindings/go/callbacks.go

gaffer.goRemove Session.c() and route all session/debug calls through uintptr-based C shims +106/-44

Remove Session.c() and route all session/debug calls through uintptr-based C shims

• Eliminates transient Go-side uintptr->*C.gaffer_session conversions by adding static cgo shims that accept uintptr_t and cast in C. Updates session lifecycle, feed/state/result/debug APIs, and callback registration call sites to use the integer handle end-to-end on the Go side.

bindings/go/gaffer.go

Tests (1) +9 / -7

runtime_test.goClarify concurrent-GC stress test explanation for stack-copy invalid-pointer failure +9/-7

Clarify concurrent-GC stress test explanation for stack-copy invalid-pointer failure

• Updates the stress-test comment to reflect the real failure mode: stack copies can occur mid-FFI call when callbacks re-enter Go, making pointer-typed slots holding integer handles fatal. Keeps the test intent as a probabilistic regression check alongside the deterministic type guard.

bindings/go/runtime_test.go

Documentation (1) +5 / -0

bindings-gc-crash.mdAdd patch changeset describing the Go bindings GC crash fix +5/-0

Add patch changeset describing the Go bindings GC crash fix

• Adds a release note documenting the rare but fatal Go GC crash and the mitigation. Explains that handles now remain integer-typed on the Go side with casting performed only in C shims, and notes downstream impact (e.g., CLI embedding the bindings).

.changeset/bindings-gc-crash.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. UB callback cast ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new registration shims cast Go callback symbols (declared with a uintptr_t userData parameter)
to the runtime’s callback typedefs that take void* user_data, which is an incompatible
function-pointer type. Calling a function through an incompatible function-pointer type is undefined
behavior in C and may miscompile or fail on some targets/toolchains.

Code

bindings/go/callbacks.go[R18-34]

+// Registration shims: cast the integer handle to gaffer_session* (and back
+// into the user_data slot) in C, so no fake pointer transits Go stack slots.
+static void go_on_emit(uintptr_t s) {
+	gaffer_on_emit((gaffer_session*)s, (gaffer_emit_cb)goEmitCallback, (void*)s);
+}
+static void go_on_log(uintptr_t s) {
+	gaffer_on_log((gaffer_session*)s, (gaffer_log_cb)goLogCallback, (void*)s);
+}
+static void go_on_diagnostic(uintptr_t s) {
+	gaffer_on_diagnostic((gaffer_session*)s, (gaffer_diagnostic_cb)goDiagnosticCallback, (void*)s);
+}
+static void go_on_state_changed(uintptr_t s) {
+	gaffer_on_state_changed((gaffer_session*)s, (gaffer_state_changed_cb)goStateChangedCallback, (void*)s);
+}
+static void go_on_break(uintptr_t s) {
+	gaffer_on_break((gaffer_session*)s, (gaffer_break_cb)goBreakCallback, (void*)s);
+}

Evidence

The runtime callback types in gaffer.h require void* user_data, but the new shim registers
goEmitCallback (declared with uintptr_t userData) by casting it to gaffer_emit_cb, creating an
incompatible function-pointer call site (UB). The exported Go callback signature indeed uses
C.uintptr_t, confirming the mismatch is real and introduced by the new shims.

bindings/go/callbacks.go[7-34]
runtime/include/gaffer.h[75-104]
bindings/go/callbacks_export.go[16-38]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`bindings/go/callbacks.go` registers callbacks by casting `goEmitCallback`/`goLogCallback`/etc to `gaffer_*_cb` function-pointer types, but the runtime typedefs take `void* user_data` while the exported Go functions take `uintptr_t userData`. This is undefined behavior in C.

## Issue Context
- Runtime API expects callback signatures with `void* user_data`.
- Go exports intentionally use `C.uintptr_t` to keep integer handles out of pointer-typed Go stack slots.

## Fix Focus Areas
- Implement C-level wrapper functions with the exact `gaffer_*_cb` signatures (taking `void* user_data`) that internally call the exported Go callbacks by converting `user_data` to `uintptr_t`.
- Register these wrappers without casting function pointers.

### Suggested approach (sketch)
- In the cgo preamble, add wrappers like:
 - `static void go_emit_cb(const char*..., void* ud) { goEmitCallback(..., (uintptr_t)ud); }`
 - Repeat for log/diagnostic/state_changed/break.
- Update `go_on_emit` etc to call `gaffer_on_emit((gaffer_session*)s, go_emit_cb, (void*)s);` (no cast).

## Fix Focus Areas (files/lines)
- bindings/go/callbacks.go[7-34]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---