Restore the AgentProcess a thread already held, rather than clearing it

[jasperblues]

Fixes #1911.

ExecutorAsyncer set the AgentProcess thread local on the worker and cleared it in a finally. Clearing is right for a pooled worker that arrived empty, and wrong the moment a task runs on a thread that is already inside a process — the submitting thread then comes back from async() holding no process.

Nothing throws at that point. The next AgentProcess.get() returns null, usually a blackboard read some distance away, so it presents as "the blackboard lost my object" with the cause several frames back.

AgentProcessAccessor gains with(), which saves and restores. reset() stays for the callers that want the old semantics.

Who is affected — narrower than it first looks

Neither the default configuration nor virtual threads can hit this bug. (Said badly in an earlier revision as "not the default, and not virtual threads", which read to at least one reviewer as a claim about what we run on. It is not — it is about which executors the defect can occur on. Embabel and assistant both use virtual threads.) threading.shared is false by default, so AsyncConfiguration builds Embabel's own executor: newCachedThreadPool on platform threads, newThreadPerTaskExecutor on virtual. Neither can run a task on the submitting thread, so neither was ever affected.

Reaching the defect needs both: embabel.agent.platform.threading.shared=true, AND an application executor that can run a task on the submitting thread — CallerRunsPolicy with a bounded queue, or a direct executor. Out of the box, nobody hits this.

Worth fixing anyway: sharing the app's executor is a supported option, and a propagation primitive that quietly empties a thread local costs whoever meets it a day.

A step-by-step walkthrough of how it goes wrong is in the comment below. To be explicit, since the walkthrough reads like an incident report and is not one: no incident prompted this PR. There was no saturated pool and no observed failure. It was found by reading ExecutorAsyncer while working on the roles SPI (#1889), and the walkthrough is a constructed scenario whose preconditions are stated in its first line.

Testing

ExecutorAsyncerCallerThreadTest, twelve cases in four groups:

• DirectExecutor — the shape in miniature: plain, throwing, nested two deep, and repeated 50x to show the caller's process is not eroded.
• CallerRunsUnderLoad — how it is actually reached: a one-worker pool with a one-slot queue and CallerRunsPolicy, saturated so the third task overflows onto the caller. Asserts the precondition (the task really did run on the submitting thread) before asserting the outcome, so it cannot pass vacuously.
• OwnedExecutorsNeverRunOnTheCaller — the boundary: virtual thread-per-task and cached platform pool, each asserting the task ran on a different thread and the caller kept its process. These pass before and after the fix, which is the point of them.
• PooledThreadIsolation — the behaviour the clearing was there to protect, asserted unchanged: a worker never carries one task's process into the next, two processes do not bleed across reuse, and parallelMap gives every worker the caller's process while leaving none behind. This is what stops the fix being "just stop cleaning up".

Five of the twelve fail on unfixed main and pass after — the four direct-executor cases plus the caller-runs one. The other seven pass both before and after; each was run against unfixed main to confirm that.

Verification run:

• new suite 12/12; 28/28 across the asyncer + accessor tests repeated 5x — stable every run
• embabel-agent-api module: 4019 tests, 0 failures
• full reactor: BUILD SUCCESS, 0 failures in any module

Follow-up, not in this PR

AgentProcess is a bare ThreadLocal propagated by hand in exactly one class, so it reaches only the threads the platform starts through Asyncer — not application threads. Micrometer's context-propagation is already a dependency and already used in this same method for observations. Registering it as a ThreadLocalAccessor on the global ContextRegistry would propagate it at every micrometer-aware boundary (Reactor, Spring AI's reactive chains, ContextPropagatingTaskDecorator) and let the hand-rolled capture/restore go away. Worth its own issue.

🤖 Generated with Claude Code

[igordayen]

@jasperblues - apparently serious issue. would be very helpful to have hand-written (not test) scenatio as real case. please consider adding to the last test AND into the issue.

Executor - Thread1 - Agent Process.....

And what is behavopr with virtual threads?

Thanks

[igordayen]

Added by mistake?

[igordayen]

nice syntax!

[alexheifetz]

Serious issue, requires rigorous testing. Let's add to post 1.5.0 release, so it can be tested more thoroughly.

[jasperblues]

Two review questions answered: a step-by-step scenario, and virtual threads. The second one narrows the scope of this PR, so I've corrected the description above.

Does this affect virtual threads? No — and not the platform default either

threading.shared defaults to false, so AsyncConfiguration builds Embabel's own executor:

• virtual threads on → Executors.newThreadPerTaskExecutor(...) — always starts a new thread
• platform threads (the default) → Executors.newCachedThreadPool(...) — always hands off to a worker

Neither can run a task on the thread that submitted it, so neither was ever affected. The clearing this PR replaces was harmless on both.

Two tests now pin that (OwnedExecutorsNeverRunOnTheCaller), one per executor, each asserting the task genuinely ran on a different thread before asserting the caller kept its process. Both pass before this PR's fix as well as after — that is the claim, so they are only meaningful if verified against unfixed main, which they were.

So reaching the defect requires two things together: embabel.agent.platform.threading.shared=true, AND an application executor that can run a task on the submitting thread — CallerRunsPolicy with a bounded queue, or a direct/synchronous executor. Out of the box, nobody hits this. My original description said "appears under load and not in development" without that precondition, which overstated it.

It is still worth fixing: sharing the app's executor is a supported, documented option, and a propagation primitive that quietly empties a thread local is the kind of thing that costs somebody a day.

The scenario, step by step

Setup: threading.shared=true, and the application's executor has a bounded queue with CallerRunsPolicy. The system is under enough load that the queue is full.

• Time 1 — A request arrives on HTTP thread http-7. Agent process P starts. On http-7, AgentProcess.get() returns P.
• Time 2 — An action runs. A guardrail writes an object onto P's blackboard. Still http-7. Works.
• Time 3 — Platform code calls asyncer.async { ... } from http-7 — a parallel tool call, a sub-process, anything going through Asyncer.
• Time 4 — Every worker is busy and the queue is full. CallerRunsPolicy therefore runs the task on http-7 itself. This is the only unusual step, and it only happens when saturated.
• Time 5 — ExecutorAsyncer captured P from the calling thread, and now sets P on the "worker" thread. The worker thread is http-7, which already had P. Nothing visibly changes.
• Time 6 — The task body runs and sees P. Correct so far.
• Time 7 — The task finishes. The finally calls reset(), which removes the value instead of putting back what was there. http-7 now has no process. Nothing throws.
• Time 8 — Control returns to the action, still on http-7, still inside the same request, still believing it is inside P.
• Time 9 — An interceptor further along calls AgentProcess.get() to read the blackboard. It gets null.
• Time 10 — It reports the object missing, or NPEs, or silently skips.

The damage is done at Time 7 and shows up at Time 9, on a different code path, with nothing logged in between. And because Time 4 only happens when the pool is saturated, the same request succeeds whenever a worker happens to be free — so it is intermittent and load-dependent.

With the fix, Time 7 restores P instead of clearing it, and Time 9 sees P.

What is unchanged

PooledThreadIsolation asserts the property the clearing existed for, and it passes before and after: a pooled worker never carries one task's process into the next, two processes never bleed across thread reuse, and parallelMap leaves nothing behind. The fix is "restore instead of clear", not "stop cleaning up".

[jasperblues]

@igordayen — right, EmbabelObjectMapperHolder.kt was not mine. A git add -A swept up a license header the Maven license plugin had written into that file while the build ran. Nothing to do with this change. Removed: force-pushed a rebuilt branch, now three files, all of them ones this PR actually touches.

(That file does still lack its header on main — a leftover from #1887. Trivial, but it belongs in its own change, not smuggled in here.)

Also: the scenario and the virtual-threads answer are in the comment above, and the timeline is now in the issue as well, as you asked.

[jasperblues]

@alexheifetz — no objection to holding it until after 1.5.0; sequencing is your call. Two things that may be useful in deciding.

Scope is narrower than it first reads. With the default configuration this is unreachable. threading.shared is false by default, so the platform builds its own executor — a cached pool, or thread-per-task on virtual threads — and neither can run a task on the submitting thread. It needs threading.shared=true plus an application executor that can (CallerRunsPolicy with a bounded queue, or a direct executor), and then only while that pool is saturated. It is also not a regression: the behaviour has been there since #1477.

On testing. Agreed this is the part that has to be right, so the suite is built to be falsifiable rather than confirmatory:

• The tests were written against unfixed main first. 5 of 12 fail there and pass after — the four direct-executor cases and the caller-runs one. The other 7 pass both before and after, and each was run against unfixed main to confirm that, so they are pinning existing behaviour rather than describing the patch.
• PooledThreadIsolation asserts the property the old clearing existed for — a worker never carrying one task's process into the next, no bleed across thread reuse, parallelMap leaving nothing behind. If the fix were "stop cleaning up", those go red.
• OwnedExecutorsNeverRunOnTheCaller covers virtual and cached-pool, each asserting the task genuinely ran on a different thread before asserting the outcome, so they cannot pass vacuously.
• CallerRunsUnderLoad asserts its precondition (the overflow task really did run on the submitting thread) before asserting the result, for the same reason.

Verification: new suite 12/12; 28/28 across the asyncer and accessor tests repeated 5x for flakiness, stable every run; embabel-agent-api 4019 tests, 0 failures; full reactor green.

If there is a specific scenario you want covered before it lands — an IT-level one, or a shape I have not thought of — name it and I will add it.

[igordayen]

@jasperblues per my understanding scenario is the edge one, when all application threads are busy. I would enhance documentation with note: use two pools with caution, increase size as needed. For Assistant switch to virtual threads. Thanks
@alexheifetz FYI

[jasperblues]

@igordayen — but pool sizing changes how often the task lands on the submitting thread. It doesn't change what happens when it does: ExecutorAsyncer clears a thread local it didn't set, so the caller comes back from async() with no AgentProcess. A larger pool makes that rarer and no less silent.

Thinking about the end user: better that it never happens than that they think "I should have read the small print" after a production crash.

[jasperblues]

@igordayen — taking the docs note and the load test. Added SharedExecutorCallerRunsWiringIT, which runs with the IT suite per #1577 and needs no keys: it boots the real AsyncConfiguration with threading.shared=true against a bounded CallerRunsPolicy pool, saturates it, and asserts the overflow genuinely ran on the submitting thread before asserting the outcome. It fails on the unfixed code with expected: <AgentProcess> but was: <null> and passes with it. A second case pins that shared=false — the default — never reaches that shape at all.

On the extra guard — I think it is already there, structurally rather than as configuration. Restore differs from clear only when the calling thread already held a process, which is precisely the caller-runs case; on every other path it restores nothing and behaves exactly as before. A flag would add a configuration in which the defect is still reachable, and a predicate that can itself be wrong. PooledThreadIsolation pins the unchanged half.

[igordayen]

@jasperblues @alexheifetz

Please consider adding Tests:

with restores previous process after success
with restores previous process after exception
with restores outer process after nested with
with clears process after block when previous process was null
with null value leaves current process unchanged, or explicitly test whatever behavior is intended

The last one matters because current code says:

if (value == null) {
return block()
}

So with(null) does not clear. That is okay if intentional, but the test should pin it.

I agree we should not add the shared-executor guard conditions.

Docs To Add In This PR
Add a short note to the threading model docs, not a long warning.

Location:

embabel-agent-docs/src/main/asciidoc/reference/asynch-mode/page.adoc

Place it near the threading.shared explanation.

Suggested text:

NOTE: embabel.agent.platform.threading.shared=true means Embabel may use the application's
applicationTaskExecutor when the application and Embabel use the same threading model. If that executor is
bounded and uses CallerRunsPolicy, saturated executor workers and a full queue can cause Embabel async work
to run on the submitting application thread. Embabel preserves AgentProcess context in this case, but this
configuration still couples agent execution to application executor backpressure and may affect request
latency. Use executor sharing deliberately.

Follow-Up After This PR (I can take it):
Create a separate issue for the config guard.

Bare minimum guard work later:

Add property:
embabel.agent.platform.threading.shared-executor-caller-runs-policy=warn

Values:
allow, warn, fail

Apply only when sharedExecutor != null.

Warn or fail for:
ThreadPoolExecutor + CallerRunsPolicy
Detect known caller-thread executors.
Warn that custom executors may also run tasks on caller threads and cannot always be detected.

Tests for that follow-up:

shared=true + CallerRunsPolicy + warn starts and logs warning
shared=true + CallerRunsPolicy + fail fails startup
shared=false + CallerRunsPolicy + fail starts because Embabel does not share it
shared=true + normal executor + fail starts

Please run all IT tests.
Thanks for the important discovery.

[jasperblues]

@igordayen — taken all of it. Tests, the docs note, and the follow-up left to you.

with tests

Added to AgentProcessAccessorTest, which already existed alongside ExecutorAsyncerContextPropagationTest and covered getValue/setValue/reset but nothing on with. Six tests, in a nested With class:

• restores the previous process after the block returns
• restores the previous process when the block throws
• restores the outer value after a nested with, not just the innermost — asserts at every level on the way out, not only the last
• clears the slot when the thread held nothing to begin with — the case clearing was written for
• a null value leaves a process the thread already holds in place
• a null value leaves an empty thread empty, and still runs the block

ExecutorAsyncerCallerThreadTest covers the same guarantee through the executor, which is where it is reached and why it matters. These state the contract on its own terms, so a change to with fails against its own rules rather than against the behaviour of a saturated thread pool several layers away.

with(null)

You are right, and it is intentional. A null value is not "clear the process for the duration" — it means the caller had nothing to propagate, which is what ExecutorAsyncer passes when a task is submitted from outside any process. The block then runs against whatever the running thread already holds, and with neither sets nor clears. Now pinned in both directions, and said on the method.

Mutation-checked rather than assumed. Reverting the finally to a bare reset() fails four of them; making with(null) clear fails the two null-value tests.

Docs

Your text verbatim, in reference/asynch-mode/page.adoc next to the threading.shared table, before the Asyncer paragraph.

Tests run

SharedExecutorCallerRunsWiringIT green (2/2), and the full suite green on this branch — 2852 tests, 0 failures.

Follow-up

Agreed on leaving the config guard out of this PR — take it, and the property shape you sketched reads right to me.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[igordayen]

@jasperblues - looks good, thank you

[igordayen]

@Jasper, the only thing I would like to get clarity on is the use case.
If it is a shared bounded executor with CallerRunsPolicy, or virtual threads. Thanks