Skip to content

Tool invoke implementation - #3787

Open
vigoo wants to merge 11 commits into
mainfrom
tool-invoke
Open

Tool invoke implementation#3787
vigoo wants to merge 11 commits into
mainfrom
tool-invoke

Conversation

@vigoo

@vigoo vigoo commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Resolves GOL-35

@vigoo
vigoo requested a review from a team August 29, 2026 09:55
@netlify

netlify Bot commented Aug 29, 2026

Copy link
Copy Markdown

Deploy Preview for golemcloud canceled.

Name Link
🔨 Latest commit 621b75a
🔍 Latest deploy log https://app.netlify.com/projects/golemcloud/deploys/6a96b8198aba3700081809e8

vigoo added 9 commits August 31, 2026 09:32
Amp-Thread-ID: https://ampcode.com/threads/T-01a05674-075d-72eb-888b-20e8037e82b1
Co-authored-by: Amp <amp@ampcode.com>

# Conflicts:
#	golem-worker-executor-test-utils/src/lib.rs
#	golem-worker-executor/src/durable_host/concurrent/call.rs
#	golem-worker-executor/src/durable_host/concurrent/mod.rs
#	golem-worker-executor/src/durable_host/p3/http/response_body.rs
#	sdks/moonbit/golem_sdk/tool/pkg.generated.mbti
#	sdks/moonbit/golem_sdk_example1/golem.yaml
#	sdks/moonbit/golem_sdk_tools/lib/tools_emit.mbt
#	sdks/scala/codegen/src/main/scala/golem/codegen/rpc/ToolRpcCodegen.scala
#	sdks/scala/codegen/src/test/scala/golem/codegen/pipeline/CodegenPipelineSpec.scala
#	sdks/scala/core/js/src/main/scala/golem/runtime/tool/ToolImplementationRuntime.scala
#	sdks/scala/model/src/main/scala/golem/tool/ToolClient.scala
#	sdks/scala/model/src/main/scala/golem/tool/ToolInvoke.scala
#	sdks/scala/model/src/main/scala/golem/tool/ToolStreams.scala
#	sdks/scala/model/src/test/scala/golem/tool/ToolClientSpec.scala
Amp-Thread-ID: https://ampcode.com/threads/T-01a05674-075d-72eb-888b-20e8037e82b1
Co-authored-by: Amp <amp@ampcode.com>

# Conflicts:
#	sdks/moonbit/golem_sdk/gen/interface/golem/tool/guest/ffi.mbt
#	sdks/moonbit/golem_sdk/gen/interface/golem/tool/guest/top.mbt
#	sdks/moonbit/golem_sdk/interface/golem/tool/host/ffi.mbt
#	sdks/moonbit/golem_sdk/interface/golem/tool/host/pkg.generated.mbti
#	sdks/moonbit/golem_sdk/interface/golem/tool/host/top.mbt
#	sdks/moonbit/golem_sdk_example1/golem_moonbit_examples/golem_tool_clients.mbt
#	sdks/moonbit/golem_sdk_example1/golem_moonbit_examples/golem_tools.mbt
#	sdks/moonbit/golem_sdk_tools/lib/tools_emit.mbt
#	sdks/moonbit/golem_sdk_tools/lib/tools_emit_test.mbt
.clone();
let remaining = dst.remaining(store.as_context_mut());
if finish {
attachment.consumer_cancel();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

finish = true means the read was cancelled, not that the stream closed

The fork documents StreamResult::Cancelled as "the operation was interrupted … and the producer may be able to produce more items" (futures_and_streams.rs:588-595), and retains the producer afterwards (:2414). The flag comes only from a guest stream.cancel-read (:4172) — reader drop is a separate path, already handled by Drop for AttachmentConsumer.

Here it's treated as terminal: consumer_cancel() clears the buffer and selects the immutable ConsumerCancelled terminal, and finished = true makes every later poll return Dropped. So one cancelled read silently discards buffered stdout and kills the attachment for good.

Same at :1087; the stdin side (tool/mod.rs:226, :305) has a worse variant, noted separately.

}

if finish {
self.items.take();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same finish = true misreading on stdin — here it becomes silent truncation

Dropping the sender ends the pump's recv() loop (:349), which falls through to producer.finish() (:364) — ByteStreamCloseCause::Finished, a clean EOF. So a guest that cancels one pending stream.write into its stdin source hands the tool a truncated input that looks like normal end-of-stream, and the invocation completes and records a success terminal.

Same at :305 in UnderlyingToolStdinStreamConsumer. See the note on attachment.rs:979 for why Cancelled isn't a close.

)
.await;
}
let host_resource_exhausted = stdin

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Replay can re-derive stdin exhaustion and hard-diverge from the recorded terminal

The preflight at :2410 only short-circuits when the recorded terminal is Skipped. A call recorded as Executed falls through to here — and host_resource_exhausted traces back to ActiveAgents::try_acquire, which is global node headroom after eviction, not a function of the invocation's inputs.

So recovering a previously-successful capable call on a loaded node synthesizes a Skipped/ResourceExhausted terminal, which then fails the equality check in complete_without_body_access (entity.rs:571) with unexpected_oplog_entry — non-retriable. Co-tenant memory pressure at recovery time permanently breaks recovery of already-durable work.

Tests can't catch it: ActiveAgents::admission is None under test, so try_acquire always grants.

Suggestion: skip this branch when scope().mode() == ReplayingCompleted.

"tool invocation was fenced before pre-dispatch cancellation"
));
}
return cancel_registered_tool_before_body(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pre-dispatch cancellation runs before the attachments are configured, so publication no-ops

This returns at :2517, above the match filesystem block at :2527 that calls configure_live() / configure_completion() — so both attachments are still AttachmentMode::Pending. cancel_registered_tool_before_body then does cancel() + publish_completion() (:2208), but publish_completion bails unless the mode is already Completion { published: false } (attachment.rs:264).

Net effect: the terminal is selected but never made visible, so the caller's stream<byte-stream-item> for stdout never yields an item and never closes — poll_produce sees chunks_visible = false, terminal_visible = false and parks. Trigger: async-invoke-and-await with stdout against an incapable tool, cancelled before dispatch.

SkippedToolAttachmentEndpoints::publish_failure (:1968) has the right shape — configure_completion() first, then fail, then publish.

The existing cancellation coverage reads a stdout chunk before cancelling (rust-caller:687), so it only exercises the post-dispatch path where the mode is already Live.

(OwnerRuntime::Agent, None) => golem_common::model::card::EffectiveSurface::default(),
(OwnerRuntime::Entity(_), _) => configured_agent_effective_surface,
};
let local_live_tail = matches!(runtime, OwnerRuntime::Entity(_)) && replay_state.is_live();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

local_live_tail is seeded from the cursor's position, not from this store's execution mode

replay_state.is_live() here is the positional check (last_replayed_index == replay_target), and the flag is a one-way latch — 11 store(true) sites, no store(false). So an entity Store constructed while the shared cursor is transiently clamped (e.g. a NonPrimary repair calling CursorTx::switch_to_live) latches true even when its own scope.mode() is ReplayingCompleted, and is_live() (:10379) then returns true for the rest of its life.

Such a body takes live paths everywhere, never claims its recorded descendants, and re-appends them at the oplog tail under the old parent Start.

The mode is known exactly at construction — is there a reason not to seed from scope.mode() (live ⇒ true, either replay mode ⇒ false) rather than from the shared cursor?

invoke-and-await: async func(
command-path: list<string>,
input: typed-schema-value,
stdin: option<stream<u8>>,
stdin: option<own<tool-stdin>>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A manually created, unfinished stdin deadlocks a synchronous invoke-and-await

The host stages capable stdin to its terminal before admitting the body, so if a caller passes a tool-stdin from create-stdin and intends to write after the call, it blocks: the host waits for the stdin terminal while the guest is blocked inside this function, and there's no cancellation path for the synchronous mode.

The SDKs avoid it by always routing through create-stdin-from-stream, but the raw contract permits the shape. Worth saying so here, next to create-stdin.

let chunk = source.read(65536) catch {
error => {
let failure : Result[FixedArray[Byte], @toolHost.ByteStreamFailure] = Err(
@toolHost.ByteStreamFailure::Failed(repr(error)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Every source failure is re-encoded as failed, losing the variant

Cancelled, Abandoned and ResourceExhausted all become ByteStreamFailure::Failed(repr(error)), so the caller can't distinguish a cancelled source from an exhausted one. This is the only place across the four SDKs where a byte-stream-failure identity is dropped — Rust, TS (isRpcError) and Scala (both directions) all map the variants exhaustively.

}

///|
pub fn ProviderStdout::output(self : ProviderStdout) -> @async-core.Sink[Byte] {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A MoonBit tool can't fail its stdout

ProviderStdout exposes output (:89), finish (:94) and drop (:113), but nothing wrapping tool-stdout-writer.fail. So a MoonBit tool can only ever select finished — or abandoned by dropping — and can never report failed(msg), resource-exhausted or cancelled on its output, even though the WIT and the host both support it. Rust exposes this as OutputStream::fail (tool_impl.rs:80-86).

buf.write_string(
" let " +
arg.source +
" = match __golem_stdin {\n Some(__golem_stream) => __golem_stream\n None => return Err(@toolCommon.ToolError::InvalidInput(\"required stdin stream is missing\"))\n }\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These emitted error returns skip the resource-release helper

The None => return Err(...) emitted here (and at :2017 for stdout) returns directly, bypassing @tool.reject_decoded_invocation, which every other error path in the generated function calls (see the emissions at :2001 and :2044). So canonical input resources are never released and a live __golem_stdin is never dropped. The pre-PR stdin branch did call it.

} from "@golemcloud/golem-ts-sdk";
import { z } from "zod/v4";

const streamingTool = toolDefinition("ts-streaming").body((body) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typescript_generated_client_streams_live doesn't use the generated client

golem.yaml:20-22 declares dependencies.tools: golem-it:tool-streaming-ts-provider/ts-streaming, which is what drives the TS bridge generator. But this re-declares the tool shape by hand (:10-16) and calls client(streamingTool) at :49, so the invocation goes through a locally-built definition rather than the generated bridge.

The Rust counterpart does the opposite — rust-caller/src/lib.rs:1,14 import StreamingClient / CapableStreamingClient from the generated crates. So despite the test name, bridge_gen/typescript/tool.rs — which this PR reworks substantially — has no end-to-end coverage.

Coverage is also much narrower than the Rust pair: one method, one command, required stdin + required stdout, one mode. No fire-and-forget, cancel, optional-stream combinations, mid-stream failure, or capable tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants