Repository navigation
Give mirror delivery a retry path, and record the creates it cannot confirm - #4
Merged
Merged
Conversation
added 2 commits
August 10, 2026 18:42
…confirmed GitHub write failures were classified as retryable and then never acted on: worker.ts computed the flag, logged it, and let guard swallow the error, so a 502 lost the mirror update outright. Writes now retry three times with a jittered 1s/4s backoff on the failures that can succeed on a second attempt (429, rate-limited 403, 5xx, and a worker->host call that timed out without an answer), honouring retry-after / x-ratelimit-reset when GitHub names a wait. A 401/404/422 still fails on the first attempt. The retry stays inside the 30s a single call already gets, from the SDK's callHost timer and the host's own AbortController. github.ts now documents that budget, and why a plugin-side AbortSignal is unreachable: ctx.http.fetch serializes only method, headers and body, so the signal never leaves the worker. Creating the mirrored issue is the one step that cannot be repeated safely, and event delivery gives no help - the host pushes events as a fire-and-forget notification and drops them when the worker is down, with no replay. So intent is now written before the POST: a mirror-create entity keyed companyId:issueId, pending until the number is stored, then done. A cron job every five minutes resolves records left open - number known becomes done, no number after a grace window becomes uncertain, logged once and never attempted again. That last state is deliberate. Confirming an ambiguous create would mean reading GitHub back, which this plugin does not do; the drain reads plugin state and entities only. So a silent duplicate becomes a recorded, visible "we do not know". The outbox lives in ctx.entities because entities can be enumerated - plugin state cannot, its list is not exposed over the worker->host RPC. Tests: 19 -> 28.
The outbox condemned every unfinished create to `uncertain`, which refuses to ever mirror that task again. That is right when the outcome is unknown — a timeout, a dead worker — because the issue may exist and a second create would duplicate it. It is wrong when GitHub answered: a 500, or a 429 that outlived the retry budget, proves nothing was created, so there is no duplicate to fear and nothing to be uncertain about. Left as it was, the outbox made this case strictly worse than the behaviour it replaced, where a failed create was simply retried on the next event. Records now split on exactly that evidence: GitHub replied -> `failed`, and a later event creates the issue; we never found out -> `pending`, and the drain condemns it after the grace window. Proved by a test that fails without the change.
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.
Closes agent-company-kit#11 — with one of its three claims withdrawn.
The issue was wrong about the deadline
ctx.http.fetchwas said to have noAbortSignaland therefore no bound. It has no signal — the SDK serializes only method/headers/body and drops it (worker-rpc-host.ts:568-596) — but the call is bounded twice anyway: the SDK's owncallHosttimer at 30s, never overridden byrunWorker, and the host'sPLUGIN_FETCH_TIMEOUT_MSwith itsAbortController. Worst case a handler parks for 30s, not for as long as the socket stays open.So no
Promise.racewrapper: it would shorten only the plugin's await while the host request keeps running, which buys nothing and reads like a fix. The 30s budget is documented ingithub.tsinstead, and the retry cap is tied to it.Retry
retryablewas computed fromGithubApiErrorand only ever logged, so a 502 or a rate-limit was written to the log and the event was gone. NowwithRetry(newsrc/retry.ts) wrapsGithubClient.request: 3 attempts, jittered 1s/4s,retry-afterandx-ratelimit-resethonoured when GitHub sends them, all inside the 30s a single call may take.The create that cannot be confirmed
Not fully closable with today's SDK, and the PR does not pretend otherwise.
ctx.stateis get/set/delete with no compare-and-set, itslistis not exposed over RPC, and event delivery is a fire-and-forget notification dropped when the worker is down — no replay. So an intent cannot be committed atomically with its result, and a create lost to a restart is simply gone.What is achievable: write the intent to
ctx.entitiesbefore the POST, close it after, and let a cron drain (*/5) resolve what is left.src/outbox.tsimports no HTTP client at all, so the drain structurally cannot call GitHub — the reconciliation trap the issue warns about is closed by construction, not by discipline.The correction that matters most
The first cut condemned every unfinished create to
uncertain, which refuses to mirror that task ever again. That is correct when the outcome is unknown. It is wrong when GitHub answered: a 500, or a 429 that outlived the budget, proves nothing was created — no duplicate to fear, nothing to be uncertain about. Left that way, the outbox would have been strictly worse than the behaviour it replaced, where a failed create was just retried on the next event.Records now split on exactly that evidence: GitHub replied →
failed, and a later event creates the issue; we never found out →pending, and the drain condemns it after the grace window. Both directions are pinned, and the regression test fails against the version without this change — verified by reverting it.Verified
pnpm run typecheckpnpm run testpnpm run buildpluginManifestV1Schemajobs.scheduleSuite still runs in ~100ms: backoff is driven by faked timers while
Date.now()stays real, so the budget check remains honest.Still open, and it is a host limitation
An
uncertainrecord is terminal: that task is never mirrored. Closing it needs either astatecompare-and-set (or a transactional state+entities write), or at-least-once event delivery with acks. Both are host-side.