Fix FFI close responsiveness and callback reclamation

[roji]

Follow up on #2610 to fix three concrete defects: Node could block its event loop for five seconds during close with pending outbound traffic, the Rust lifecycle regression depended on thread scheduling, and Java permanently retained JNA callback wrappers after successful close.

• Run Node's native connection_close through Koffi's async facility so outbound callbacks can finish on the event loop. Retain callback state and keepalive until close reports quiescence, then unregister the callback and shut down the host.
• Relax only the Rust test's initial close-call count; retain its callback lifetime, eventual reclamation, and idempotence assertions.
• Release Java's wrapper root only after successful native close, preserving it on false/exception. Keep the registration lookup through the existing shutdown-before-close ordering so a later successful close can reclaim it.

Public shutdown completion/error contracts and the graceful-stop versus force-stop distinction are unchanged. No native ABI or cross-SDK cleanup redesign is included.

Validation

• Node: focused unit/E2E tests, typecheck, ESLint, and formatting pass. Real-native E2E suite passed 10 repetitions. On macOS ARM64 with published runtime 1.0.84-4, an independent 10 ms timer was delayed 5,007 ms before the fix and 11-12 ms afterward. Public force-stop and callback-triggered disposal both reclaimed resources successfully.
• Rust: lifecycle regression passed 100 repetitions; formatting passes.
• Java: targeted mvn verify, Spotless, and Checkstyle pass. The regression exercises real JNA trampoline upcalls, failed-close retention, successful-close garbage collection, and both shutdown orderings; it passed five repeated runs. Three optional packaged-runtime loading tests were skipped because the native JAR was unavailable.

[Copilot]

Copilot review overview

🟡 Changes recommended

Node shutdown now resolves before asynchronous native cleanup finishes, changing its completion semantics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	nodejs/​src/​ffiRuntimeHost.ts — Await asynchronous FFI cleanup before reporting shutdown complete

What changed in this PR

Updates FFI shutdown behavior to keep Node responsive, stabilize Rust lifecycle testing, and reclaim Java callback wrappers.

Changes:

• Runs Node connection close asynchronously with expanded lifecycle coverage.
• Relaxes the Rust test’s scheduling-sensitive assertion.
• Releases Java callback roots after successful close.

File	Description
nodejs/​src/​ffiRuntimeHost.ts	Moves native close to Koffi’s asynchronous API.
nodejs/​test/​ffiRuntimeHost.test.ts	Tests asynchronous close and resource retention.
nodejs/​test/​e2e/​inprocess_ffi.e2e.test.ts	Verifies event-loop responsiveness during force-stop.
rust/​src/​ffi.rs	Makes the lifecycle assertion scheduling-independent.
java/​sdk/​src/​main/​java/​com/​github/​copilot/​ffi/​JnaNativeBinding.java	Releases callback roots after quiescence.
java/​sdk/​src/​test/​java/​com/​github/​copilot/​ffi/​JnaNativeBindingTest.java	Tests retention, reclamation, and shutdown ordering.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[stephentoub]

I reviewed the relevant Node and Java discussions from #2610 for context.

For Node, the five-second behavior was explicitly understood: the discussion explained that synchronous connection_close can wait for Koffi's event-loop callback relay and hit the runtime's quiescence timeout. The decision in #2610 was not to add a timing-based E2E because it would not prove callback reclamation safety; it was not an assertion that blocking the event loop was desirable. Moving close to Koffi's async facility is therefore a sound follow-up, subject to preserving shutdown completion semantics as noted inline.

For Java, process-lifetime retention of the JNA callback wrapper was also explicit and intentional in #2610—the resolution stated that the wrapper root would remain intact after detaching its delegate. Given the clarified ABI guarantee that a successful connection_close is the callback-quiescence barrier, this follow-up's narrower lifetime is valid: retain on false or exception, and release only after successful close. Preserving the registration across host-shutdown-before-close ordering also looks correct.

Generated by Copilot

[stephentoub]

The Java callback reclamation and Node async-close direction are sound in light of the #2610 discussions. Approving the overall change; please address the P2 shutdown-completion issue called out inline before merging.

Generated by Copilot

[github-actions]

SDK Consistency Review — PR #2622

This PR fixes an in-process FFI callback lifetime/quiescence bug and touches three SDKs (Java, Node.js, Rust). I compared the changed logic against the equivalent FFI host implementations in Go (go/internal/ffihost/ffihost.go), .NET (dotnet/src/FfiRuntimeHost.cs), and Python (python/copilot/_ffi_runtime_host.py).

Findings: no cross-SDK inconsistency introduced ✅

• Java (JnaNativeBinding.java): Fixes a genuine memory leak — previously RETAINED_CALLBACKS entries were held for the lifetime of the JVM and hostShutdown could prematurely detach/remove callback registrations before connectionClose reported quiescence. The fix now only releases the wrapper root after a successful connectionClose, and hostShutdown alone only detaches the Java delegate without removing the wrapper root. I verified the equivalent teardown ordering in the other four SDKs already enforces "only release/unregister the outbound callback after connectionClose returns true, never on hostShutdown alone or on failed/throwing close":

  • Go: tryFinalizeCleanupLocked only deletes from outboundTargets after a successful connectionClose.
  • .NET: TryFinalizeNativeCleanup only calls _releaseNativeCallback() after _connectionClose returns true (and quarantines on exception, not on shutdown).
  • Python: _try_finalize_cleanup only clears _outbound_callback after closed is truthy.
  • Node.js (pre-PR): unregisterCallback() was already only invoked after a successful close.

  So this was a Java-specific bug (owing to JNA's need to explicitly GC-root callback trampolines) and the fix brings Java in line with the behavior the other four SDKs already had — no further action needed elsewhere.

• Node.js (client.ts / ffiRuntimeHost.ts): dispose() is changed to async and connectionClose is now invoked via koffi's .async() variant so the JS event loop can tick while native code drains outbound callbacks before reporting quiescence. This is a Node/libuv-specific concern (single-threaded event loop) that doesn't apply to Go/.NET/Python/Rust, which use real OS threads for native callback delivery and already call connectionClose synchronously/blocking during dispose. CopilotClient call sites were correctly updated to await host.dispose() in both the graceful-stop and force-stop paths.

• Rust (ffi.rs): Only a test assertion relaxation (== 1 → >= 1 for close-call count), consistent with retry-tolerant close semantics already implied elsewhere.

Verdict

This is a targeted, language-specific bug fix confined to internal FFI plumbing (FfiRuntimeHost/JnaNativeBinding are internal/package-private, not part of the public SDK surface in any language). No public API changes, and no equivalent bug found in Go, .NET, or Python that needs the same fix. No action needed in other SDKs.

Generated by SDK Consistency Review Agent for #2622 · copilot · sonnet50 · 94.4 AIC · ⌖ 12.3 AIC · ⊞ 8.3K · ◷