Skip to content

revert(plugin): drop the per-call plugin deadline - #621

Open
JeroenSoeters wants to merge 2 commits into
mainfrom
jeroensoeters/revert-plugin-call-deadline
Open

revert(plugin): drop the per-call plugin deadline#621
JeroenSoeters wants to merge 2 commits into
mainfrom
jeroensoeters/revert-plugin-call-deadline

Conversation

@JeroenSoeters

Copy link
Copy Markdown
Collaborator

Summary

The retry-aware watchdog work bounded every watched plugin call by a deadline the agent supplied through the operator's spawn environment. This removes it and keeps the rest of that work.

The deadline never fixed the reported failure. Healthy resources were declared missing in action because one plugin RPC blocked inside its own retry loop far longer than the status-poll interval, starving the loop that produces heartbeats. Bounding that retry in the AWS plugin is what fixed it. The agent-side deadline only enforced the invariant the plugin had violated. Both workflow regression tests for the reported bug pass without it.

What it cost was disproportionate. A deadline is not an additive interface change a plugin author can ignore, the way an added request field is. It is a cross-cutting obligation that applies inside every method, is violated by omission rather than commission, fails silently, and cannot be verified by reading your own code. It also cannot be enforced: Go has no goroutine preemption, so a plugin that does not thread the context still blocks and the agent still waits. Paying that much interface surface for a guarantee that is not one is the wrong trade, particularly as plugins are increasingly written by agents that implement an interface faithfully and observe a prose invariant unreliably.

Kept

  • The watchdog window derived from the operator's actual retry cadence, including the flat RetryDelay term.
  • Status calls no longer terminating without sending a final progress, so the real error reaches the updater instead of a watchdog message a minute later.
  • The window sized from the per-plugin config the operator was really spawned with, rather than the deprecated global.

Changed

The window keeps a per-call allowance, renamed PluginCallAllowance and documented for what it actually is: the silence the agent tolerates for one call, matching the longest the updater itself waits for a reply, rather than a bound anything enforces. The agent's own outer call timeout returns to 60s, since there is no longer an operator deadline for it to outlast.

TestFailedStatusCallNeverReissuesTheOriginalOperation is kept and narrowed to the recoverable path, which is the only one that can reschedule. The plugin double it shared with the deleted deadline tests moves to plugin_operator_double_test.go.

Verification

go build ./..., the full -tags=unit suite across internal/... and pkg/plugin, and the three MIA workflow tests under -tags=integration (TestSlowHeartbeatIsNotDeclaredMissingInAction, TestFailedStatusCheckReportsThePluginError, TestPluginOperatorCrashConvergesViaTimeout) all pass. That the first two still pass without the deadline is the evidence that the window sizing, not the deadline, is what fixed the reported bug.

The retry-aware watchdog work bounded every watched plugin call by a
deadline the agent supplied through the operator's spawn environment.
Removing it again, and keeping the rest of that work.

The deadline never fixed the reported failure. Healthy resources were
declared missing in action because one plugin RPC blocked inside its own
retry loop far longer than the status-poll interval, starving the loop
that produces heartbeats. Bounding that retry in the plugin is what fixed
it; the agent-side deadline only enforced the invariant the plugin had
violated. The two workflow regression tests for the reported bug pass
without it.

What it cost was disproportionate. A deadline is not an additive
interface change a plugin author can ignore, the way an added request
field is. It is a cross-cutting obligation that applies inside every
method, is violated by omission rather than commission, fails silently,
and cannot be verified by reading your own code. It also cannot be
enforced: Go has no goroutine preemption, so a plugin that does not
thread the context still blocks, and the agent still waits. Paying that
much interface surface for a guarantee that is not actually a guarantee
is the wrong trade, especially as plugins are increasingly written by
agents that will implement an interface faithfully and observe a prose
invariant unreliably.

The watchdog window keeps a per-call allowance, now named for what it
is: the silence the agent tolerates for one call, matching the longest
the updater itself waits for a reply, rather than a bound anything
enforces.

Kept from the same work: the window derived from the operator's actual
retry cadence, the fix for status calls terminating without sending a
final progress, and sizing the window from the per-plugin config the
operator was really spawned with. The test that a failed status call
never reissues the original mutation is kept and narrowed to the
recoverable path, which is the only one that can reschedule.
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.

1 participant