fix(executor): isolate per-tool registration failures - #136
Conversation
155c4e0 to
c4f33b3
Compare
`/register/tools` failed the whole batch when a single tool's schema
couldn't be typed by our codegen (e.g. a recursive `$ref`). The portal
federates arbitrary upstream JSON Schema, so this fired constantly: in
prod each session degraded to ~135 sequential per-tool register calls
(~6.4s, ~274 daily 500s) instead of one batch call.
Fix it at the layer that owns the problem:
- Codegen never fails a tool over typing. `Tool::new` is infallible;
`generate_types` failures degrade to a permissive `any` signature
(with a warning) so the tool stays callable, just untyped.
- Registration isolates per-tool. `add_callbacks` returns a
`CallbackReport { registered, failed }` instead of bubbling the first
error; a genuinely bad tool (name clash, unparseable schema) is
skipped and reported, never aborts the batch. The handler returns 200
with the report.
The portal's existing batch call now succeeds, so its per-tool fallback
never triggers — no portal change required to fix prod.
Refs #119.
c4f33b3 to
d42790b
Compare
Elias Posen (eliasposen)
left a comment
There was a problem hiding this comment.
This hits the question "what is a failure?" when dealing with a single registration vs a batch. I am inclined to agree with the PR that a batch should not fail due to one registration but rather report what has failed, and a single registration should fail given the same tool.
I think what is missing is reporting the warnings (downgrades to any) over the wire in the session server.
| callbacks: impl IntoIterator<Item = &'a CallbackConfig>, | ||
| ) -> Result<Self> { | ||
| self.add_callbacks(callbacks)?; | ||
| self.add_callbacks(callbacks); |
There was a problem hiding this comment.
this loses the CallbackReport returned by add_callbacks such that the caller has no idea if any fail, perhaps we need to return a tuple? breaking change but might be worth it
| /// signature so it remains callable at runtime, rather than being rejected. | ||
| fn generate_types_lenient(schema: RootSchema, type_name: &str, tool: &str) -> TypegenResult { | ||
| generate_types(schema, type_name).unwrap_or_else(|e| { | ||
| warn!(tool, type_name, error = %e, "codegen failed; degrading tool type to `any`"); |
There was a problem hiding this comment.
my feeling is that this should be returned as part of the CallbackReport. Sure there is a log, but if the registration happens to pctx deployed remotely via the session server the client has no way of knowing there is a warning
Two follow-ups from review: - Codegen degradation to `any` only reached the logs, so a client registering tools against a remotely deployed session server had no way to learn its tool lost its types. `Tool` now records why each schema was degraded, `CallbackReport` carries them as `warnings`, and `/register/tools` returns them alongside `failed`. - `with_callbacks` swallowed the `CallbackReport` from `add_callbacks`, hiding both failures and warnings from the builder-style caller. It now returns `(Self, CallbackReport)`. Breaking, and infallible: per-tool isolation means the batch cannot fail, so the report is the only outcome. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
/register/toolsfailed the whole batch when a single tool's schema couldn't be typed by our codegen (e.g. attio's recursive$ref). Since we federate arbitrary upstream JSON Schema, this fired constantly.Measured in prod (7d,
gateway_calls+ Cloud Run logs): every code-mode session degraded to ~135 sequential per-toolregister/toolscalls (~6.4s, ~274 daily 500s) instead of one batch call — all round-trips through the executor's public front door. Locally the same calls are localhost, so it only hurts in the cloud.This is the #119 pattern one layer down: one bad actor takes down the rest.
Fix — at the layer that owns the problem
Tool::newis now infallible;generate_typesfailures degrade to a permissiveanysignature (with awarn!) so the tool stays callable, just untyped — never dropped.add_callbacksreturns aCallbackReport { registered, failed, warnings }instead of bubbling the first error. A genuinely bad tool (name clash, unparseable schema) is skipped and reported; the batch never aborts. The handler returns200with the report, never500on one bad tool.anyis listed in the report'swarningsand returned in the/register/toolsresponse, so a client talking to a remotely deployed session server learns its tool lost its types instead of that fact living only in our logs.API changes
CodeMode::with_callbacksreturns(Self, CallbackReport)— previously it discarded the report, hiding failures and warnings from builder-style callers. Infallible now: per-tool isolation means the batch cannot fail, so the report is the only outcome. Breaking; no in-repo callers.CodeMode::add_callbackreturnsResult<Vec<String>>— the reasons that tool's types were degraded (empty when fully typed).RegisterToolsResponsegainswarnings: Vec<CallbackWarning { id, reason }>;openapi.jsonregenerated.Why this fixes prod on its own
The portal's existing
register_resilientsends the batch first — that batch now succeeds, so its per-tool fallback never triggers. The ~135 calls collapse to one successful batch call, with no portal change required. Deleting the portal-sideregister_resilient+sanitize_input_schemabecomes a clean follow-up, not a prerequisite.Verification
cargo check --workspaceclean,cargo fmt --checkclean, clippy-clean on touched files.add_callbacks_isolates_a_failing_tool(batch survives one bad tool),add_callbacks_reports_degraded_types(degraded tool registers and is reported),tool_degrades_uncodegenable_schema_to_anyandtool_with_generatable_schema_reports_no_degradation. Existingregistrations.rsexpectations updated for the newfailedfield. All pass.Refs #119.
🤖 Generated with Claude Code