Record background-thread host calls and support nested MockHost installs#1698
Open
jax-0n-git wants to merge 1 commit into
Open
Conversation
MockHost resolved every host-API trampoline through a thread-local install slot, which had two gaps versus the behavior the kit documents: 1. Host-global callbacks (log / config_set / config_delete / dispatch / http_request) made from a plugin-spawned background thread — an explicitly supported frame per osaurus_plugin.h, and the reason the recorders are NSLock-guarded — found no thread-local slot and were silently dropped. A plugin that persists config or logs on an async path recorded nothing, so a passing assertion was a false negative. 2. The docstrings promised a thread-local install "stack" that "restores the previous host," and the trap message said to "nest with withInstalled," but hostAPIPointer trapped on any second install and uninstall unconditionally cleared the slot — nesting was impossible. Resolve host-global callbacks through a new recordingHost(): the thread's install slot if present, else the uniquely-installed host from a process-wide registry, so a background-thread call is recorded when one host is installed and dropped — never misrouted — when attribution is ambiguous. get_active_agent_id keeps using the thread-local slot so it still returns NULL outside a per-agent frame, per the ABI. hostAPIPointer now saves the displaced host and uninstall restores it, so installs nest; re-installing the same host still traps, and uninstall traps on a non-LIFO/cross-thread unwind (which would otherwise leak the displaced host and leave the slot pointing at it) — withInstalled's defer keeps the blessed path LIFO. Adds a Threading suite (nested under a now-.serialized MockHostTests so host installs never overlap) covering background-thread log/config recording, the get_active_agent_id NULL-on-background-thread rule, the ambiguous-multi-host drop (joining its worker so it can't perturb the next test), nested install/restore, a weak-reference regression that the LIFO unwind neither leaks a host nor poisons the slot, and an exit-test regression that a non-LIFO uninstall traps. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jax-0n-git
force-pushed
the
fix/mockhost-background-thread-and-nested-installs
branch
from
203b182 to
9e3772f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MockHostresolved every host-API trampoline through a thread-local install slot, which had two gaps versus the behaviorOsaurusPluginTestKitdocuments:Background-thread host calls were dropped. Host-global callbacks (
log/config_set/config_delete/dispatch/http_request) made from a plugin-spawned background thread — an explicitly supported frame perosaurus_plugin.h, and the reason the recorders areNSLock-guarded — found no thread-local slot incurrent()and were silently dropped. A plugin that persists config or logs on an async path recorded nothing, so a passinghost.configWrites.lastValue(...) != nil/host.logs.contains(...)assertion was a false negative.withInstalledcouldn't nest. The docstrings promised a thread-local install "stack" that "restores the previous host," and the precondition message said to "nest withwithInstalled," buthostAPIPointertrapped on any second install anduninstallunconditionally cleared the slot — nesting was impossible.Approach
recordingHost()resolves host-global callbacks to the calling thread's install slot if present, else — for a background thread with no slot — the uniquely-installed host from a small process-wide registry. When zero or ≥2 hosts are installed the background call is ambiguous and is dropped, never misrouted. (A@convention(c)trampoline carries no per-host context, so a contextless background call can only be attributed via process-wide state; single-host is the well-defined common case.)get_active_agent_idkeeps using the thread-local slot, so it still returnsNULLoutside a per-agent frame (including a background thread), matching the ABI.hostAPIPointersaves the displaced host anduninstallrestores it, so installs nest (LIFO, aswithInstalled'sdeferguarantees). Re-installing the same host without an interveninguninstallstill traps.Changes
Test Plan
swift testinPackages/OsaurusPluginTestKit(15 tests, all green) +swift-format lint --strictclean. NewThreadingsuite covers background-thread log/config recording, theget_active_agent_idNULL-on-background-thread rule, the ambiguous-multi-host drop, and nested install/restore. The suite is nested under a now-.serializedMockHostTestsso host installs never overlap (the background-thread fallback's single-host attribution is only well-defined when installs don't run concurrently); verified stable across 20 consecutive full-suite runs. No MLX/GUI dependency.