agent-lifecycle: keep wake dedup alive until the real wake settles - #491
Merged
Merged
Conversation
ensureAwake's pendingWakes coalescing map cleared its entry the moment a caller's own timeout fired, not when the underlying wake() call actually settled -- so a caller arriving after a timeout could dispatch a second, concurrent wake for an address whose real wake was still running. Rewrites the existing "never settles" test to assert the corrected behavior (no second dispatch, even across a third call) and adds a case where the underlying wake resolves after the first caller's timeout, proving a later caller joins it instead of starting its own.
pendingWakes.delete was chained off the timeout-bound promise, so it fired as soon as a caller's own timeout rejected -- even though the underlying wake(address) call kept running past it. A later caller then saw no in-flight entry and dispatched a second, concurrent wake for the same address, racing two redeploys against each other. Splits the untimed raw wake promise (stored in pendingWakes, cleared only when it truly settles) from a fresh per-caller timeout race every ensureAwake caller gets independently. wake() now runs at most once per address for as long as one is genuinely in flight, regardless of how many callers' timeouts fire on top of it. Trade-off, accepted deliberately: an address whose wake never settles at all stays permanently deduped until process restart, rather than retried by every later caller -- a silently wedged address is a smaller failure than concurrently redeploying it forever.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
createAgentLifecycle'sensureAwakecoalesces concurrent callers onto one in-flight wake per address via apendingWakesmap, bounded by a per-caller timeout so a wake the host never acks fails loud instead of hanging forever (CL-6643).The bug:
pendingWakes.delete(address)was chained off the timeout-bound promise, so it fired the instant a caller's own timeout rejected — not when the underlyingwake(address)call itself actually settled. A later caller for the same address then saw no in-flight entry and dispatched a second, concurrentwake()call, racing two redeploys against each other on the host.Fix
Splits the untimed "raw"
wake(address)promise (stored inpendingWakes, cleared only when it truly settles) from a fresh per-caller timeout race everyensureAwakecaller — the initiator or a later joiner — gets independently.wake()now runs at most once per address for as long as one is genuinely in flight, no matter how many callers' individual timeouts fire on top of it.Deliberate trade-off (reviewed with Greybeard before implementation): an address whose underlying
wake()never settles at all stays permanently deduped — no caller can trigger a fresh attempt until process restart. This is accepted as a smaller failure than the concurrent-wake race it closes, and is proven explicitly by a test asserting no second dispatch across three separate calls against awakethat never settles.Stack
Bottom of a two-PR stack. CL-7214 (
packages/chat's wake-coalescing bypass) is stacked on top of this branch, since it routes through the primitive this PR fixes.Review
Closes CL-7217.