Poll legacy custom action managers to a terminal status - #1074
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: Poll legacy custom action managers to a terminal statusBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness: the Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
|
The review comments here are eerily akin to those of a previous draft (#1063), so the next commit will attempt to skip some of the back-and-forth here. |
|
▎ Two deliberate choices: The poll intervals are unexported package variables rather than injected configuration — they're production constants whose only non-default consumer is this package's sequential tests, and a config parameter would add API surface to a deprecated compatibility path. And an explicitly set but unrecognized status at the invoke seam fails closed rather than passing through: only the zero value means "this manager never populated status," while any other value is a claim the wrapper refuses to misread as success. The same statuses arriving from polls get the three-strike tolerance instead, because polls repeat and transient anomalies can recover. |
|
Status classification, stated once: the wrapper sorts every invoke and poll status into three buckets. UNSPECIFIED means the manager never populated status — the legacy pass-through, resolving with the response as the old wrapper always did. COMPLETE and FAILED are settled and resolve immediately, at the seam and from polls alike — including the in-band shape where the SDK's own manager reports a handler failure as FAILED with a nil error. Everything else is unresolved: polled to a terminal status when an id is present (with a shared three-strike tolerance covering both lookup errors and indeterminate answers), passed through when there's no id to poll. One consequence to know about enum evolution: if a new terminal status is ever added to BatonActionStatus, this wrapper will treat it as unresolved — poll it to the tolerance threshold, then fail closed — until the value is added to the settled-status checks. That's safe (it can never be misread as success) but deliberate: the settled set is enumerated, not inferred. Poll pacing is captured at registration, so the detached poll goroutine reads no shared state; tests drive the loop in milliseconds through the same parameter. |
ggreer
left a comment
ggreer
left a comment
There was a problem hiding this comment.
I noticed a couple of tiny things where some new functions could be used in existing code, but LGTM.
registerLegacyAction discarded the id and status a CustomActionManager returned, so a non-terminal status resolved the outer action as complete with whatever partial response existed while the underlying action was still running. Keep the invoke response for synchronous managers that never populated id or status, and poll GetActionStatus with capped backoff for explicitly in-flight results, bounded by the handler context's deadline: tolerate a few consecutive lookup failures, carry the last real response, and fail the outer action when the inner one reports failure or an unexpected status. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A legacy manager reporting failure in-band — FAILED status with a nil error, as the SDK's own manager does for a fast-failing handler — resolved the outer action as a success at the invoke seam. Terminal statuses at the seam now resolve exactly like terminal polls, with the status checked before the id so an explicit failure without an id cannot pass through either; only a never-populated status keeps the legacy fire-and-forget behavior. The poll loop treats indeterminate statuses with the same three-strike tolerance as lookup errors, keeps only meaningful poll payloads, and reports the handler deadline's cause instead of a bare context error. The global invoke path now carries the caller's logger into the detached handler context, matching the resource path, so the loop's warnings are no longer dropped; an observer-backed test pins that. Poll intervals are variables so tests drive the loop in milliseconds: an outcome table covers the seam and poll matrices, and the original interval-driven tests shed about fifteen seconds of suite wall time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An indeterminate status at the invoke seam was failed immediately while the same status arriving from a poll got the three-strike tolerance; both now take the tolerance path. A settled seam status still resolves like a terminal poll, an unresolved claim without an id keeps the fire-and-forget pass-through, and the status helper shrinks to the two values its call sites pass. Poll pacing moves from package variables into a small struct captured at registration: the detached poll goroutine read the cap on every iteration, so test overrides of the globals were a latent data race. Tests now pass short intervals directly, and a capturing registry drives the wrapped handler under a caller-owned timeout cause to cover the context exit, asserting the budget's cause survives the wrapper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ll pacing The meaningful-payload guard in the poll loop had no test that could fail if it were removed, because every fake returned the same payload for invokes and polls. Scripted polls can now carry their own payload, and a displacement test invokes with one payload, feeds three indeterminate polls carrying another, and asserts the fail-closed exit still returns the invoke's payload. The settled-status check is one predicate now, used at the invoke seam, the poll switch, and the retention guard, so the gates cannot drift; a new terminal enum value joins there or takes the indeterminate path. Zero or negative poll intervals fall back to the defaults instead of busy-looping the status poll. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The zero-interval guard had no test that could fail if it were removed — the same gap the payload-displacement instrument closed one commit earlier. A zero-valued pacing struct now proves the fallback: the handler resolves terminally with exactly one poll, and that poll must wait the defaults' initial tick rather than firing immediately, which is the observable difference between the guard and a busy loop. A poll count alone cannot discriminate, since a spinning loop with a single scripted poll also polls exactly once. The guard's comment also covers the inverted-pair case, which needs no normalization: the cap applies from the second tick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fallback test only passed a zero-valued pacing struct, which trips the initial-interval check on its own — weakening the guard to ignore the cap left the suite green while a live initial with a zero cap busy-loops from the second tick. The test is now a table whose second case passes exactly that shape; because the fallback replaces the whole struct, the same one-poll elapsed lower bound discriminates both halves: a guarded poll waits the defaults' initial tick, an unguarded one fires at the un-defaulted initial. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The outer error replaces the response's error field, so the generic 'legacy action failed' message was destroying the inner manager's real failure text at exactly the moment it should surface. legacyStatusErr now folds the response's reported error into the outer message, at the invoke seam and from polls alike. The lookup-error threshold exit also wraps with the action name like the loop's other exits, instead of returning a bare context error, and the threshold test asserts on the wrapper text so the wrap itself is pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The settled exit read the retained response for the inner error message, so an in-flight poll's snapshot could be reported as the failure cause when the poll that settled to FAILED carried no payload of its own. The error-message source is now the settling poll's payload — nil falls back to the generic message — while the retained response remains the returned response value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1270fad to
d7d7409
Compare
|
The robo-review is not converging (basically reversing its own position), my agent's thoughts:
|
addressing these in #1080 |
2 participants
What this does
registerLegacyAction wraps actions from the deprecated CustomActionManager / RegisterActionManagerLimited interfaces as ordinary action handlers. Until now the wrapper discarded the id and status the legacy manager returned, so an action still running at the legacy manager's own short wait was resolved as complete, with whatever partial response existed — a false success for any slow legacy action.
The wrapper now:
Why it's here
Review on the inline_wait PR (#1071 and previous drafts) flagged that the legacy path discards the inner status — meaning the requested wait can't be honored for legacy managers, and slow legacy actions false-complete regardless. This change fixes that at the root rather than threading the wait into the deprecated interface: once the wrapped handler blocks to a terminal status, inline_wait composes with legacy managers with no extra plumbing (an action finishing inside the window returns its real terminal status inline; one still running returns an honest RUNNING). It also stands alone: the false-complete existed before inline_wait did.
We can choose to reject this kind of behavior change, but I think it's appropriate for us to decide whether or not to address this issue, since it's raised by bots over and over.
Behavior changes and risk
Perimeter. Only connectors on the deprecated interfaces are affected, and within those, only actions that are still in flight when the legacy manager's own wait expires. Fast legacy actions (the common case — single API calls) return a terminal status from the invoke directly, never enter the poll loop, and behave byte-for-byte as before. Modern actions don't touch this code.
The honest sharp edge: this makes previously-dead code load-bearing. Because the old wrapper discarded the status, a legacy manager's GetActionStatus has never been exercised through this path — bugs in hand-rolled implementations have been invisible. After this change, a persistently failing lookup fails the action (visibly, after three consecutive errors) and a status that never converges occupies a goroutine until the handler deadline, then fails. A visible failure is the correct replacement for a silent false success, but it can read as a regression for a fire-and-forget action that "worked" before. Mitigating this in practice: the known legacy implementations return the SDK's own ActionManager as their CustomActionManager, so the GetActionStatus being promoted to load-bearing is the SDK's own, not custom code. Reviewer ask: if you know of a connector that hand-rolls GetActionStatus, that's the one worth checking against this change.
Observable shift for slow legacy actions. Callers see an honest in-flight status at the wait boundary instead of a fast false COMPLETE, with the real outcome landing when the inner action finishes. This is deployment-order safe: standalone, the platform's current handling of in-flight responses is unchanged; combined with the inline_wait change, slow legacy actions gain real inline outcomes.
Resource bounds. Worst case is one goroutine per slow legacy invoke for the handler budget, and a handful of status calls under the 30s backoff cap — the same order as any slow modern handler.
Testing
Five new tests cover the wrapper: synchronous managers resolve unchanged, in-flight results poll to success and to failure, consecutive lookup errors fail closed at the threshold, and a transient error followed by recovery succeeds. Note the poll intervals are production constants, so these tests contribute several seconds of real wall time to the connectorbuilder suite.