Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,21 @@ under the License.
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<version>3.4.2</version>
<executions>
<execution>
<goals>
<goal>test-jar</goal>
</goals>
</execution>
</executions>
</plugin>
</plugins>
</build>

</project>
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@ public interface RunnerContext {
*/
<T> T durableExecuteAsync(DurableCallable<T> callable) throws Exception;

/** Creates a new session id for a sub-agent call. */
String nextSessionId();

/** Creates a new call id for a sub-agent invocation under the given session. */
String nextCallId(String sessionId);

/** Clean up the resource. */
void close() throws Exception;
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ public enum ResourceType {
PROMPT("prompt"),
TOOL("tool"),
MCP_SERVER("mcp_server"),
SKILLS("skills");
SKILLS("skills"),
AGENT("agent");

private final String value;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.flink.agents.api.subagent;

import org.apache.flink.agents.api.context.DurableCallable;

/**
* Convenience base for the {@link DurableCallable} returned by {@code asAsyncCallable}.
*
* <p>Keys the durable call by the framework-assigned identity as {@code sessionId#callId} (the
* {@link SubagentSetup} contract) and captures exceptions thrown by {@link #callInternal()} into
* {@link Result#error(Exception)}, so failures are reported through the result rather than thrown.
* Implementations only provide {@link #callInternal()}.
*/
public abstract class BaseSubagentCallable implements DurableCallable<Result> {

private final String sessionId;
private final String callId;

protected BaseSubagentCallable(String sessionId, String callId) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this from the perspective of someone writing an external integration against the new surface. BaseSubagentCallable is the convenience base the API steers implementations to (:23-29, and subagent.py:199-201 says so explicitly). It does not override DurableCallable#reconciler(), so every sub-agent callable inherits the null default at DurableCallable.java:74.

durableExecute selects the reconcile state machine only when reconciler() is non-null (RunnerContextImpl.java:267-275), and durableExecuteAsync, which is the path SubagentSetup.call takes, gates identically (JavaRunnerContextImpl.java:62-70). Either way sub-agent calls land on durableExecuteCompletionOnly. On that path appendPendingCall is never reached: its only callers are inside durableExecuteWithReconcile (:555, :561). A crash between "external agent invoked" and "result persisted" therefore leaves no record at all, replay misses the cache, and call() re-invokes the external agent.

Grepping the new surface, reconcil does not appear anywhere under api/.../subagent/, in the e2e tests, or in the runtime sub-agent tests. Python surfaces the field (subagent.py:82) and forwards it (:184), but wires None and no test asserts the forwarding. The recovery tests seed only terminal CallResults (SubagentIdentityRecoveryTest.java:105-108,171-178), so the pending path has no sub-agent coverage on either side.

Concretely: what does an integration author write today to get reconcile-before-resend, and is there anything on this surface that would tell them the option exists? One shape that would make the choice visible, in case it is useful: BaseSubagentCallable taking the reconciler as a constructor argument, so passing null is something the author decided rather than a default they never saw.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseSubagentCallable doesn't shield any DurableCallable convention — it inherits reconciler() as-is (default null) and only simplifies boilerplate (getId, getResultClass, call with exception capture). Without a reconciler, a crash between invocation and persistence causes replay to re-invoke the external agent — this is expected, identical to any DurableCallable, and typically fine for read-only or idempotent sub-agents. If side effects must not be replayed, the author should provide a reconciler per the existing DurableCallable contract.

The question is whether to surface this option more visibly. Here are three options:

  1. Add reconcilerInternal to BaseSubagentCallable — simplest, but just restates a convention DurableCallable already defines.
  2. Remove BaseSubagentCallable — if the abstraction confuses developers into thinking it shields DurableCallable contracts, maybe we should remove it makes them implement DurableCallable directly.
  3. Default reconciler + abstract querySuccess() — if the typical pattern is if (querySuccess()) return result; else return callInternal(), we can provide a default reconciler() that uses it. Enables the pending/reconcile path; developers can still override.

Which do you prefer?

this.sessionId = sessionId;
this.callId = callId;
}

@Override
public String getId() {
return sessionId + "#" + callId;
}

@Override
public Class<Result> getResultClass() {
return Result.class;
}

@Override
public final Result call() {
try {
return Result.ok(callInternal());
} catch (Exception e) {
return Result.error(e);
}
}

/**
* Performs the invocation and returns the JSON-serializable payload. Thrown exceptions are
* captured into a failed {@link Result}.
*/
protected abstract Object callInternal() throws Exception;
}
98 changes: 98 additions & 0 deletions api/src/main/java/org/apache/flink/agents/api/subagent/Result.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.flink.agents.api.subagent;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;

import java.io.PrintWriter;
import java.io.Serializable;
import java.io.StringWriter;

/**
* Outcome of a {@link Subagent} call.
*
* <p>Sub-agent implementations should capture internal failures into a {@code Result} (via {@link
* #error}) instead of throwing, so callers can inspect {@link #isSuccess()} without try/catch.
*
* <p>The failure cause is carried as a serializable {@code errorMessage} — the full stack trace of
* the failure — rather than a live exception, so that a {@code Result} can be persisted through
* durable execution.
*/
public class Result implements Serializable {

private static final long serialVersionUID = 1L;

private final boolean success;
private final Object result;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result is typed Object, BaseSubagentCallable.getResultClass() pins the durable result class to Result.class (BaseSubagentCallable.java:47-48), and recovery re-binds through the plain OBJECT_MAPPER at RunnerContextImpl.java:516, which is constructed with no polymorphic typing (:67-68).

I round-tripped a small POJO payload through this Result with writeValueAsString / readValue(s, Result.class):

serialized: {"success":true,"result":{"verdict":"approve","score":7},"errorMessage":null}
payload class after replay: java.util.LinkedHashMap
ClassCastException: class java.util.LinkedHashMap cannot be cast to class Review

So getResult() hands back the author's type on the first execution and a LinkedHashMap after a failover replay. The shipped example casts at ExternalSubagentAgent.java:52 (((List<?>) result.getResult()).get(0)) and survives only because JSON arrays bind to ArrayList. Every payload in the suite is a String or a List<String> (MockExternalSubagentSetup.java:92, SubagentIdentityRecoveryTest.java:108), so nothing currently exercises the shape that breaks.

Python does not diverge here. Its durable payload goes through cloudpickle (flink_runner_context.py:430,473), which preserves the type, so this is also a Java/Python semantic gap on new public API that AGENTS.md asks to keep aligned.

What should getResult() return after a replay when the sub-agent returned a record or a POJO? A couple of routes, in case they help: making Result generic and threading the payload class through getResultClass(), or keeping the field opaque and adding getResult(Class<T>) backed by OBJECT_MAPPER.convertValue. Either way a test with a non-String, non-collection payload would pin the behavior.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this — it's a real oversight. I'm currently working on the cross-language sub-agent invocation design and ran into the same issue there.

The plan is to add getResult(Class<T>) backed by OBJECT_MAPPER.convertValue. This unifies all three paths where result can appear as a LinkedHashMap: durable recovery (Jackson JSON deserialization), cross-language sub-agent calls (pemja conversion), and first execution (direct cast, no conversion needed). convertValue handles the Map→POJO conversion uniformly regardless of the source.

One known limitation: for generic collection payloads like List, both recovery and cross-language paths leave result as ArrayList — getResult(List.class) only returns List, losing element types. A getResult(TypeReference<T>) overload can be added later if type-safe collections are needed.

private final String errorMessage;

@JsonCreator
public Result(
@JsonProperty("success") boolean success,
@JsonProperty("result") Object result,
@JsonProperty("errorMessage") String errorMessage) {
this.success = success;
this.result = result;
this.errorMessage = errorMessage;
}

/** Creates a successful result carrying the given value. */
public static Result ok(Object result) {
return new Result(true, result, null);
}

/** Creates a failed result carrying the full stack trace of the given exception. */
public static Result error(Exception exception) {
return new Result(false, null, exception == null ? null : stackTraceOf(exception));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error(Exception) stores the full stack trace as errorMessage, and BaseSubagentCallable.call() captures every exception into it rather than throwing (BaseSubagentCallable.java:52-58). Two things follow downstream.

The durable layer sees a normal completion: durableExecuteCompletionOnly calls recordDurableCompletion with a null exception (RunnerContextImpl.java:329), and CallResult's status is derived as exceptionPayload == null ? SUCCEEDED : FAILED (CallResult.java:100). So isFailure() (:177-179) is false for every failed sub-agent call, and getCurrentCallResultFields() reports "SUCCEEDED" (RunnerContextImpl.java:490). Anything keyed on durable-call status reads sub-agent failures as successes.

The trace is also uncapped, and recordCallCompletion persists the ActionState immediately to the configured store (RunnerContextImpl.java:827-837, backed by KafkaActionStateStore / FlussActionStateStore). A flapping external agent writes a multi-KB string per failure into durable storage.

Worth capping what gets persisted, say the message plus the top N frames, and keeping the full trace to the log? And is collapsing a captured failure into a SUCCEEDED CallResult deliberate, or should the two stay distinguishable at that layer?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is by design. As discussed in #909, "sub-agent implementations should intercept internal exceptions and populate Result, without directly exposing them to the caller." The intent is to prevent sub-agent exceptions from directly affecting the caller while avoiding the need for every caller to write its own try-catch. When an exception occurs, the persisted Result carries the failure info; the caller always receives a failed Result rather than the exception itself, so Action behavior stays consistent between first run and replay.

On durable-layer status (SUCCEEDED for failed sub-agent calls): This is deliberate. SUCCEEDED at the durable layer means "the durable call completed and returned a Result" — the sub-agent's success or failure is carried inside the Result payload (result.isFailure()), which is a separate concern from durable execution status. On replay, the caller reads the same failed Result from the persisted resultPayload, so processing behavior matches the first run.

On uncapped stack trace: The current text-based exception (full stack trace as errorMessage) was chosen to ensure cross-language transmission. I also felt the full stack trace is more helpful for problem analysis, and since exceptions are typically low-frequency events, storage savings wasn't a primary concern. While I also agree that for the caller the stack trace isn't critical — it's sufficient to find it in logs. I can align with the current convention: persist only exception type + message, and log the full stack trace.

If we were to switch to using DurableExecutionException directly, besides the cross-language issue, there's also a pre-existing problem where the recovered exception (RuntimeException from toException()) differs from the original exception type on first execution — catch blocks targeting specific exception types would no longer match after recovery. This is unrelated to sub-agent; we can discuss it separately.

Does the by-design approach (capturing into Result rather than throwing) seem reasonable, or do you prefer requiring callers to try-catch?

}

/** Creates a failed result carrying the given message. */
public static Result error(String errorMessage) {
return new Result(false, null, errorMessage);
}

private static String stackTraceOf(Exception exception) {
StringWriter writer = new StringWriter();
exception.printStackTrace(new PrintWriter(writer));
return writer.toString();
}

public boolean isSuccess() {
return success;
}

public Object getResult() {
return result;
}

public String getErrorMessage() {
return errorMessage;
}

/**
* Reconstructs an exception carrying the stored stack trace as its message, or null if this
* result is successful.
*/
@JsonIgnore
public Exception getException() {
return success ? null : new RuntimeException(errorMessage);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.flink.agents.api.subagent;

import org.apache.flink.agents.api.context.DurableCallable;
import org.apache.flink.agents.api.context.RunnerContext;

/**
* Caller-facing interface for all sub-agents (external and internal).
*
* <p>An invocation is identified by a {@code (sessionId, callId)} pair; the session groups a
* conversation across invocations. Both ids are assigned by the framework ({@link
* RunnerContext#nextSessionId()} and {@link RunnerContext#nextCallId(String)}): callers may supply
* a session id to continue a prior session but never supply a call id.
*/
public interface Subagent {

/** Synchronously invokes the sub-agent, creating a new session. */
Result call(RunnerContext ctx, Object prompt) throws Exception;

/** Synchronously invokes the sub-agent continuing {@code sessionId}. */
Result call(RunnerContext ctx, Object prompt, String sessionId) throws Exception;

/** Produces a deferred, durable callable for this sub-agent call, creating a new session. */
DurableCallable<Result> asAsyncCallable(RunnerContext ctx, Object prompt);

/**
* Produces a deferred, durable callable for this sub-agent call continuing {@code sessionId}.
*/
DurableCallable<Result> asAsyncCallable(RunnerContext ctx, Object prompt, String sessionId);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.flink.agents.api.subagent;

import com.fasterxml.jackson.annotation.JsonIgnore;
import org.apache.flink.agents.api.context.DurableCallable;
import org.apache.flink.agents.api.context.RunnerContext;
import org.apache.flink.agents.api.resource.ResourceType;
import org.apache.flink.agents.api.resource.SerializableResource;

/**
* Base setup for an external sub-agent resource. Serialized into the agent plan as an {@code AGENT}
* resource.
*
* <p>Hosts the id-resolution chain behind {@link Subagent}: omitted ids are assigned via the
* context before an implementation is ever invoked, and {@link #call} runs the deferred callable
* through durable execution. Implementations only provide the terminal {@code asAsyncCallable}.
*/
public abstract class SubagentSetup extends SerializableResource implements Subagent {

@Override
@JsonIgnore
public ResourceType getResourceType() {
return ResourceType.AGENT;
}

@Override
public Result call(RunnerContext ctx, Object prompt) throws Exception {
return call(ctx, prompt, ctx.nextSessionId());
}

@Override
public Result call(RunnerContext ctx, Object prompt, String sessionId) throws Exception {
return call(ctx, prompt, sessionId, ctx.nextCallId(sessionId));
}

/**
* Synchronously invokes the sub-agent with already-assigned {@code sessionId} and {@code
* callId}, running the deferred callable through durable execution. Framework-facing: the ids
* are assigned by the shorter variants, never supplied by callers.
*/
public Result call(RunnerContext ctx, Object prompt, String sessionId, String callId)
throws Exception {
return ctx.durableExecuteAsync(asAsyncCallable(ctx, prompt, sessionId, callId));
}

@Override
public DurableCallable<Result> asAsyncCallable(RunnerContext ctx, Object prompt) {
return asAsyncCallable(ctx, prompt, ctx.nextSessionId());
}

@Override
public DurableCallable<Result> asAsyncCallable(
RunnerContext ctx, Object prompt, String sessionId) {
return asAsyncCallable(ctx, prompt, sessionId, ctx.nextCallId(sessionId));
}

/**
* Produces the deferred, durable callable for one invocation; both ids are already assigned.
* The only method implementations must provide. Contract: the returned {@link
* DurableCallable#getId()} MUST be derived solely from the {@code (sessionId, callId)} pair.
*/
protected abstract DurableCallable<Result> asAsyncCallable(
RunnerContext ctx, Object prompt, String sessionId, String callId);
}
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ public static LoadedFile buildAgents(Path path) {
addSharedDescriptors(
sharedResources, ResourceType.VECTOR_STORE, doc.getVectorStores(), path);
addSharedDescriptors(sharedResources, ResourceType.MCP_SERVER, doc.getMcpServers(), path);
addSharedDescriptors(sharedResources, ResourceType.AGENT, doc.getSubagents(), path);

for (ToolSpec t : doc.getTools()) {
if (sharedResources.get(ResourceType.TOOL).put(t.getName(), buildTool(t)) != null) {
Expand Down Expand Up @@ -390,6 +391,7 @@ private static Agent buildAgent(AgentSpec spec) {
addAgentDescriptors(agent, ResourceType.EMBEDDING_MODEL, spec.getEmbeddingModelSetups());
addAgentDescriptors(agent, ResourceType.VECTOR_STORE, spec.getVectorStores());
addAgentDescriptors(agent, ResourceType.MCP_SERVER, spec.getMcpServers());
addAgentDescriptors(agent, ResourceType.AGENT, spec.getSubagents());

for (ToolSpec t : spec.getTools()) {
agent.addResource(t.getName(), ResourceType.TOOL, buildTool(t));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public final class AgentSpec {
private final List<DescriptorSpec> embeddingModelSetups;
private final List<DescriptorSpec> vectorStores;
private final List<DescriptorSpec> mcpServers;
private final List<DescriptorSpec> subagents;

@JsonCreator
public AgentSpec(
Expand All @@ -55,7 +56,8 @@ public AgentSpec(
List<DescriptorSpec> embeddingModelConnections,
@JsonProperty("embedding_model_setups") List<DescriptorSpec> embeddingModelSetups,
@JsonProperty("vector_stores") List<DescriptorSpec> vectorStores,
@JsonProperty("mcp_servers") List<DescriptorSpec> mcpServers) {
@JsonProperty("mcp_servers") List<DescriptorSpec> mcpServers,
@JsonProperty("subagents") List<DescriptorSpec> subagents) {
this.name = name;
this.description = description;
this.prompts = orEmpty(prompts);
Expand All @@ -68,6 +70,7 @@ public AgentSpec(
this.embeddingModelSetups = orEmpty(embeddingModelSetups);
this.vectorStores = orEmpty(vectorStores);
this.mcpServers = orEmpty(mcpServers);
this.subagents = orEmpty(subagents);
}

private static <T> List<T> orEmpty(List<T> list) {
Expand Down Expand Up @@ -121,4 +124,8 @@ public List<DescriptorSpec> getVectorStores() {
public List<DescriptorSpec> getMcpServers() {
return mcpServers;
}

public List<DescriptorSpec> getSubagents() {
return subagents;
}
}
Loading
Loading