fix: avoid NoSuchElementException in dynamic tool refresh on evicted user message (#5820)
Closes #5819 ### Problem With a dynamic `ToolProvider` (e.g. `Skills.toolProvider()` with skill-scoped tools), a multi-round tool call over a bounded chat memory throws partway through the tool loop instead of completing: ``` java.util.NoSuchElementException: No value present at dev.langchain4j.service.tool.ToolService.refreshDynamicProviders(ToolService.java:...) ``` ### Cause `refreshDynamicProviders()` recovered the user message with `UserMessage.findLast(messages).orElseThrow()`. It runs on every round, so once `MessageWindowChatMemory` evicts the original `UserMessage`, `messages` holds only `AiMessage`/`ToolExecutionResultMessage` and `orElseThrow()` throws. This hits both the blocking and streaming loops (they share the method); static `.tools()` are unaffected because only dynamic providers are refreshed (the method returns early when there are none). ### Fix Take the user message from `invocationContext.userMessage()`, falling back to `findLast` and skipping the refresh if neither exists. This is the same source the module already uses elsewhere: the initial `ToolProviderRequest` in `createContextFromStaticToolsAndProviders` is built from it, and both loops use it for `replaceLast` `refreshDynamicProviders` was the only place recovering the user message by scanning `messages`. It is the current invocation's message and is immune to eviction, so in the normal case it equals the previous value and behavior is unchanged. No public API change. Happy to fail fast instead of skipping if you prefer. ### Tests All fail on `main`, pass with the fix: - `ToolServiceTest` — unit tests on `refreshDynamicProviders`. - `DynamicToolProviderMemoryEvictionTest` — end-to-end, blocking and streaming. - `SkillsStreamingMemoryEvictionTest` — the reported setup. ``` mvn -pl langchain4j test # 1270 passed mvn -pl langchain4j-skills test # 91 passed ``` Note: Spotless (`ratchetFrom`) reformats the whole file on first touch, so `ToolService.java` has some unrelated formatting churn; the functional change is only the block above. --------- Co-authored-by: Dmytro Liubarskyi <ljubarskij@gmail.com>
This commit is contained in:
parent
987d58ebe2
commit
4ffca63871
|
|
@ -0,0 +1,84 @@
|
|||
package dev.langchain4j.skills;
|
||||
|
||||
import static java.util.concurrent.TimeUnit.SECONDS;
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
|
||||
import dev.langchain4j.agent.tool.Tool;
|
||||
import dev.langchain4j.agent.tool.ToolExecutionRequest;
|
||||
import dev.langchain4j.data.message.AiMessage;
|
||||
import dev.langchain4j.memory.chat.MessageWindowChatMemory;
|
||||
import dev.langchain4j.model.chat.mock.StreamingChatModelMock;
|
||||
import dev.langchain4j.model.chat.response.ChatResponse;
|
||||
import dev.langchain4j.service.AiServices;
|
||||
import dev.langchain4j.service.TokenStream;
|
||||
import java.util.concurrent.CompletableFuture;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
class SkillsStreamingMemoryEvictionTest {
|
||||
|
||||
interface Assistant {
|
||||
TokenStream chat(String userMessage);
|
||||
}
|
||||
|
||||
static class InventoryTools {
|
||||
|
||||
@Tool("Queries the internal inventory system for stock levels")
|
||||
String query_inventory() {
|
||||
return "47 units in stock";
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
void should_complete_when_user_message_evicted_from_memory_window_during_tool_loop() throws Exception {
|
||||
|
||||
Skill skill = Skill.builder()
|
||||
.name("inventory-management")
|
||||
.description("Describes how to query and manage the internal inventory system")
|
||||
.content("When asked about inventory or stock levels, use the 'query_inventory' tool.")
|
||||
.tools(new InventoryTools())
|
||||
.build();
|
||||
|
||||
Skills skills = Skills.from(skill);
|
||||
|
||||
StreamingChatModelMock streamingChatModelMock = StreamingChatModelMock.thatAlwaysStreams(
|
||||
AiMessage.from(ToolExecutionRequest.builder()
|
||||
.name("activate_skill")
|
||||
.arguments("{\"skill_name\":\"inventory-management\"}")
|
||||
.build()),
|
||||
AiMessage.from(ToolExecutionRequest.builder()
|
||||
.name("query_inventory")
|
||||
.arguments("{}")
|
||||
.build()),
|
||||
AiMessage.from(ToolExecutionRequest.builder()
|
||||
.name("query_inventory")
|
||||
.arguments("{}")
|
||||
.build()),
|
||||
AiMessage.from(ToolExecutionRequest.builder()
|
||||
.name("query_inventory")
|
||||
.arguments("{}")
|
||||
.build()),
|
||||
AiMessage.from("There are 47 units in stock."));
|
||||
|
||||
Assistant assistant = AiServices.builder(Assistant.class)
|
||||
.streamingChatModel(streamingChatModelMock)
|
||||
.chatMemory(MessageWindowChatMemory.withMaxMessages(3))
|
||||
.systemMessage("You have access to the following skills:\n" + skills.formatAvailableSkills()
|
||||
+ "\nActivate the relevant skill first using the 'activate_skill' tool.")
|
||||
.toolProvider(skills.toolProvider())
|
||||
.build();
|
||||
|
||||
ChatResponse response = chat(assistant, "Check the inventory for widgets");
|
||||
|
||||
assertThat(response.aiMessage().text()).contains("47 units");
|
||||
}
|
||||
|
||||
private static ChatResponse chat(Assistant assistant, String userMessage) throws Exception {
|
||||
CompletableFuture<ChatResponse> future = new CompletableFuture<>();
|
||||
assistant
|
||||
.chat(userMessage)
|
||||
.onCompleteResponse(future::complete)
|
||||
.onError(future::completeExceptionally)
|
||||
.start();
|
||||
return future.get(60, SECONDS);
|
||||
}
|
||||
}
|
||||
|
|
@ -12,9 +12,9 @@ import static dev.langchain4j.internal.Utils.isNullOrBlank;
|
|||
import static dev.langchain4j.service.IllegalConfigurationException.illegalConfiguration;
|
||||
|
||||
import dev.langchain4j.Internal;
|
||||
import dev.langchain4j.agent.tool.CompensateFor;
|
||||
import dev.langchain4j.agent.tool.P;
|
||||
import dev.langchain4j.agent.tool.ReturnBehavior;
|
||||
import dev.langchain4j.agent.tool.CompensateFor;
|
||||
import dev.langchain4j.agent.tool.Tool;
|
||||
import dev.langchain4j.agent.tool.ToolExecutionRequest;
|
||||
import dev.langchain4j.agent.tool.ToolMemoryId;
|
||||
|
|
@ -43,12 +43,10 @@ import dev.langchain4j.service.IllegalConfigurationException;
|
|||
import dev.langchain4j.service.tool.search.ToolSearchService;
|
||||
import dev.langchain4j.service.tool.search.ToolSearchStrategy;
|
||||
import java.lang.reflect.Method;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
import java.lang.reflect.Parameter;
|
||||
import java.time.LocalDateTime;
|
||||
import java.util.Arrays;
|
||||
import java.util.ArrayList;
|
||||
import java.util.Arrays;
|
||||
import java.util.Collection;
|
||||
import java.util.HashMap;
|
||||
import java.util.LinkedHashMap;
|
||||
|
|
@ -63,6 +61,8 @@ import java.util.concurrent.Executor;
|
|||
import java.util.function.BiConsumer;
|
||||
import java.util.function.Consumer;
|
||||
import java.util.function.Function;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
|
||||
@Internal
|
||||
public class ToolService {
|
||||
|
|
@ -267,8 +267,7 @@ public class ToolService {
|
|||
}
|
||||
}
|
||||
|
||||
private Map<String, BiConsumer<ToolExecution, InvocationContext>> findCompensatingActions(
|
||||
Object objectWithTools) {
|
||||
private Map<String, BiConsumer<ToolExecution, InvocationContext>> findCompensatingActions(Object objectWithTools) {
|
||||
Map<String, BiConsumer<ToolExecution, InvocationContext>> compensatingActions = new HashMap<>();
|
||||
if (compensatingToolMisconfiguration != null) {
|
||||
return compensatingActions;
|
||||
|
|
@ -300,15 +299,17 @@ public class ToolService {
|
|||
}
|
||||
Method toolMethod = ((DefaultToolExecutor) toolExecutor).originalMethod();
|
||||
Class<?>[] compensatingParams = method.getParameterTypes();
|
||||
boolean acceptsToolExecution = compensatingParams.length == 1
|
||||
&& compensatingParams[0] == ToolExecution.class;
|
||||
if (!acceptsToolExecution
|
||||
&& !Arrays.equals(toolMethod.getParameterTypes(), compensatingParams)) {
|
||||
boolean acceptsToolExecution =
|
||||
compensatingParams.length == 1 && compensatingParams[0] == ToolExecution.class;
|
||||
if (!acceptsToolExecution && !Arrays.equals(toolMethod.getParameterTypes(), compensatingParams)) {
|
||||
compensatingToolMisconfiguration = illegalConfiguration(
|
||||
"@CompensateFor(\"%s\") on method '%s.%s' must have the same parameter types as tool '%s'"
|
||||
+ " or a single %s parameter",
|
||||
toolName, objectWithTools.getClass().getName(), method.getName(),
|
||||
toolName, ToolExecution.class.getSimpleName());
|
||||
toolName,
|
||||
objectWithTools.getClass().getName(),
|
||||
method.getName(),
|
||||
toolName,
|
||||
ToolExecution.class.getSimpleName());
|
||||
if (compensateOnToolErrors) {
|
||||
throw compensatingToolMisconfiguration;
|
||||
}
|
||||
|
|
@ -331,8 +332,9 @@ public class ToolService {
|
|||
.methodToInvoke(method)
|
||||
.propagateToolExecutionExceptions(true)
|
||||
.build();
|
||||
compensatingActions.put(toolName, (toolExecution, ctx) ->
|
||||
executor.executeWithContext(toolExecution.request(), ctx));
|
||||
compensatingActions.put(
|
||||
toolName,
|
||||
(toolExecution, ctx) -> executor.executeWithContext(toolExecution.request(), ctx));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -667,23 +669,24 @@ public class ToolService {
|
|||
.build();
|
||||
}
|
||||
|
||||
private void rewriteCurrentResults(List<ToolExecutionRequest> toolExecutionRequests,
|
||||
Map<ToolExecutionRequest, ToolExecutionResult> toolResults,
|
||||
List<ToolExecutionResultMessage> resultMessages,
|
||||
String failedToolName) {
|
||||
private void rewriteCurrentResults(
|
||||
List<ToolExecutionRequest> toolExecutionRequests,
|
||||
Map<ToolExecutionRequest, ToolExecutionResult> toolResults,
|
||||
List<ToolExecutionResultMessage> resultMessages,
|
||||
String failedToolName) {
|
||||
for (int i = 0; i < toolExecutionRequests.size(); i++) {
|
||||
ToolExecutionRequest request = toolExecutionRequests.get(i);
|
||||
if (!toolResults.get(request).isError()
|
||||
&& compensatingExecutors.containsKey(request.name())) {
|
||||
if (!toolResults.get(request).isError() && compensatingExecutors.containsKey(request.name())) {
|
||||
resultMessages.set(i, rolledBackResultMessage(resultMessages.get(i), failedToolName));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private static void rewriteChatMemoryForCompensatedTools(List<ChatMessage> messages,
|
||||
ChatMemory chatMemory,
|
||||
List<CompensableToolExecution> compensableExecutions,
|
||||
String failedToolName) {
|
||||
private static void rewriteChatMemoryForCompensatedTools(
|
||||
List<ChatMessage> messages,
|
||||
ChatMemory chatMemory,
|
||||
List<CompensableToolExecution> compensableExecutions,
|
||||
String failedToolName) {
|
||||
List<ChatMessage> memoryMessages = chatMemory != null ? new ArrayList<>(chatMemory.messages()) : messages;
|
||||
for (CompensableToolExecution entry : compensableExecutions) {
|
||||
ToolExecutionResultMessage originalMsg = entry.resultMessage();
|
||||
|
|
@ -713,7 +716,8 @@ public class ToolService {
|
|||
|
||||
private record CompensableToolExecution(ToolExecution toolExecution, ToolExecutionResultMessage resultMessage) {}
|
||||
|
||||
private void compensateToolsActions(List<CompensableToolExecution> compensableExecutions, InvocationContext invocationContext) {
|
||||
private void compensateToolsActions(
|
||||
List<CompensableToolExecution> compensableExecutions, InvocationContext invocationContext) {
|
||||
for (int i = compensableExecutions.size() - 1; i >= 0; i--) {
|
||||
ToolExecution toolExecution = compensableExecutions.get(i).toolExecution();
|
||||
String toolName = toolExecution.request().name();
|
||||
|
|
@ -760,7 +764,14 @@ public class ToolService {
|
|||
return toolServiceContext;
|
||||
}
|
||||
|
||||
UserMessage userMessage = UserMessage.findLast(messages).orElseThrow();
|
||||
UserMessage userMessage = invocationContext.userMessage();
|
||||
if (userMessage == null) {
|
||||
userMessage = UserMessage.findLast(messages).orElse(null);
|
||||
}
|
||||
if (userMessage == null) {
|
||||
return toolServiceContext;
|
||||
}
|
||||
|
||||
ToolProviderRequest request = ToolProviderRequest.builder()
|
||||
.invocationContext(invocationContext)
|
||||
.userMessage(userMessage)
|
||||
|
|
|
|||
|
|
@ -0,0 +1,88 @@
|
|||
package dev.langchain4j.service.tool;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
|
||||
import dev.langchain4j.agent.tool.ToolExecutionRequest;
|
||||
import dev.langchain4j.agent.tool.ToolSpecification;
|
||||
import dev.langchain4j.data.message.AiMessage;
|
||||
import dev.langchain4j.memory.chat.MessageWindowChatMemory;
|
||||
import dev.langchain4j.model.chat.mock.ChatModelMock;
|
||||
import dev.langchain4j.model.chat.mock.StreamingChatModelMock;
|
||||
import dev.langchain4j.model.chat.response.ChatResponse;
|
||||
import dev.langchain4j.service.AiServices;
|
||||
import dev.langchain4j.service.TokenStream;
|
||||
import java.util.concurrent.CompletableFuture;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
class DynamicToolProviderMemoryEvictionTest {
|
||||
|
||||
interface Assistant {
|
||||
String chat(String userMessage);
|
||||
}
|
||||
|
||||
interface StreamingAssistant {
|
||||
TokenStream chat(String userMessage);
|
||||
}
|
||||
|
||||
private static final ToolProvider ALWAYS_TIME_PROVIDER = new ToolProvider() {
|
||||
|
||||
@Override
|
||||
public ToolProviderResult provideTools(ToolProviderRequest request) {
|
||||
return ToolProviderResult.builder()
|
||||
.add(
|
||||
ToolSpecification.builder()
|
||||
.name("get_time")
|
||||
.description("Returns the current time")
|
||||
.build(),
|
||||
(req, memoryId) -> "12:00")
|
||||
.build();
|
||||
}
|
||||
|
||||
@Override
|
||||
public boolean isDynamic() {
|
||||
return true;
|
||||
}
|
||||
};
|
||||
|
||||
private static AiMessage callTime() {
|
||||
return AiMessage.from(
|
||||
ToolExecutionRequest.builder().name("get_time").arguments("{}").build());
|
||||
}
|
||||
|
||||
@Test
|
||||
void blocking_tool_loop_survives_user_message_eviction() {
|
||||
|
||||
ChatModelMock model = ChatModelMock.thatAlwaysResponds(callTime(), callTime(), AiMessage.from("It is 12:00."));
|
||||
|
||||
Assistant assistant = AiServices.builder(Assistant.class)
|
||||
.chatModel(model)
|
||||
.chatMemory(MessageWindowChatMemory.withMaxMessages(2))
|
||||
.toolProvider(ALWAYS_TIME_PROVIDER)
|
||||
.build();
|
||||
|
||||
assertThat(assistant.chat("What time is it?")).contains("12:00");
|
||||
}
|
||||
|
||||
@Test
|
||||
void streaming_tool_loop_survives_user_message_eviction() throws Exception {
|
||||
|
||||
StreamingChatModelMock model =
|
||||
StreamingChatModelMock.thatAlwaysStreams(callTime(), callTime(), AiMessage.from("It is 12:00."));
|
||||
|
||||
StreamingAssistant assistant = AiServices.builder(StreamingAssistant.class)
|
||||
.streamingChatModel(model)
|
||||
.chatMemory(MessageWindowChatMemory.withMaxMessages(2))
|
||||
.toolProvider(ALWAYS_TIME_PROVIDER)
|
||||
.build();
|
||||
|
||||
CompletableFuture<ChatResponse> future = new CompletableFuture<>();
|
||||
assistant
|
||||
.chat("What time is it?")
|
||||
.onCompleteResponse(future::complete)
|
||||
.onError(future::completeExceptionally)
|
||||
.start();
|
||||
|
||||
assertThat(future.get(60, TimeUnit.SECONDS).aiMessage().text()).contains("12:00");
|
||||
}
|
||||
}
|
||||
|
|
@ -1,13 +1,22 @@
|
|||
package dev.langchain4j.service.tool;
|
||||
|
||||
import static dev.langchain4j.service.tool.ToolService.executeWithErrorHandling;
|
||||
import static dev.langchain4j.service.tool.ToolService.refreshDynamicProviders;
|
||||
import static dev.langchain4j.service.tool.ToolService.shouldReturnImmediately;
|
||||
import static org.assertj.core.api.Assertions.*;
|
||||
|
||||
import dev.langchain4j.agent.tool.ToolExecutionRequest;
|
||||
import dev.langchain4j.agent.tool.ToolSpecification;
|
||||
import dev.langchain4j.data.message.AiMessage;
|
||||
import dev.langchain4j.data.message.ChatMessage;
|
||||
import dev.langchain4j.data.message.ToolExecutionResultMessage;
|
||||
import dev.langchain4j.data.message.UserMessage;
|
||||
import dev.langchain4j.exception.ToolExecutionException;
|
||||
import dev.langchain4j.invocation.InvocationContext;
|
||||
import java.util.ArrayList;
|
||||
import java.util.HashMap;
|
||||
import java.util.List;
|
||||
import java.util.concurrent.atomic.AtomicReference;
|
||||
import java.util.function.Consumer;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
|
|
@ -158,4 +167,69 @@ class ToolServiceTest {
|
|||
assertThat(toolService.beforeToolExecution()).isNull();
|
||||
assertThat(toolService.afterToolExecution()).isNull();
|
||||
}
|
||||
|
||||
// --- refreshDynamicProviders ---
|
||||
|
||||
private static ToolServiceContext contextWith(ToolProvider dynamicProvider) {
|
||||
return ToolServiceContext.builder()
|
||||
.effectiveTools(new ArrayList<>())
|
||||
.availableTools(new ArrayList<>())
|
||||
.toolExecutors(new HashMap<>())
|
||||
.returnBehaviors(new HashMap<>())
|
||||
.dynamicToolProviders(List.of(dynamicProvider))
|
||||
.build();
|
||||
}
|
||||
|
||||
@Test
|
||||
void refreshDynamicProviders_uses_invocation_user_message_when_evicted_from_messages() {
|
||||
List<ChatMessage> messages = List.of(
|
||||
AiMessage.from(ToolExecutionRequest.builder()
|
||||
.name("previous")
|
||||
.arguments("{}")
|
||||
.build()),
|
||||
ToolExecutionResultMessage.from("id", "previous", "result"));
|
||||
|
||||
UserMessage currentUserMessage = UserMessage.from("do the thing");
|
||||
InvocationContext invocationContext =
|
||||
InvocationContext.builder().userMessage(currentUserMessage).build();
|
||||
|
||||
AtomicReference<UserMessage> seenByProvider = new AtomicReference<>();
|
||||
ToolProvider dynamicProvider = request -> {
|
||||
seenByProvider.set(request.userMessage());
|
||||
return ToolProviderResult.builder()
|
||||
.add(ToolSpecification.builder().name("dynamic_tool").build(), (req, memoryId) -> "ok")
|
||||
.build();
|
||||
};
|
||||
|
||||
ToolServiceContext refreshed =
|
||||
refreshDynamicProviders(contextWith(dynamicProvider), messages, invocationContext);
|
||||
|
||||
assertThat(seenByProvider.get()).isEqualTo(currentUserMessage);
|
||||
assertThat(refreshed.toolExecutors()).containsKey("dynamic_tool");
|
||||
assertThat(refreshed.effectiveTools())
|
||||
.extracting(ToolSpecification::name)
|
||||
.contains("dynamic_tool");
|
||||
}
|
||||
|
||||
@Test
|
||||
void refreshDynamicProviders_returns_context_unchanged_when_no_user_message_available() {
|
||||
List<ChatMessage> messages = List.of(
|
||||
AiMessage.from(ToolExecutionRequest.builder()
|
||||
.name("previous")
|
||||
.arguments("{}")
|
||||
.build()),
|
||||
ToolExecutionResultMessage.from("id", "previous", "result"));
|
||||
|
||||
InvocationContext invocationContext = InvocationContext.builder().build();
|
||||
|
||||
ToolProvider dynamicProvider = request -> ToolProviderResult.builder()
|
||||
.add(ToolSpecification.builder().name("dynamic_tool").build(), (req, memoryId) -> "ok")
|
||||
.build();
|
||||
|
||||
ToolServiceContext context = contextWith(dynamicProvider);
|
||||
ToolServiceContext refreshed = refreshDynamicProviders(context, messages, invocationContext);
|
||||
|
||||
assertThat(refreshed).isSameAs(context);
|
||||
assertThat(refreshed.toolExecutors()).doesNotContainKey("dynamic_tool");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue