fix(review): replace the live in-flight dispatch check with a persisted R2 marker - #4133
Merged
Conversation
…ed R2 marker The prior fix queried GitHub's own runs API right before dispatching, which has a real gap: a freshly-dispatched run isn't guaranteed to be visible via that API the instant dispatch returns, so a poll landing in that window could still redispatch and cancel the in-flight run. A persisted marker written synchronously on a successful dispatch, cleared on the webhook_run completion (any conclusion), and self-expiring after the workflow's 15-minute timeout closes that gap without depending on API eventual consistency.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4133 +/- ##
=======================================
Coverage 93.71% 93.71%
=======================================
Files 385 385
Lines 36195 36207 +12
Branches 13263 13265 +2
=======================================
+ Hits 33919 33932 +13
Misses 1618 1618
+ Partials 658 657 -1
🚀 New features to boost your workflow:
|
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
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
hasInFlightFallbackDispatch's live GitHub runs-API query has a real eventual-consistency gap — a freshly-dispatched run isn't guaranteed to be visible via that API the instant dispatch returns, so a recapture-poll landing in that narrow window could still redispatch and get itself cancelled by the workflow's ownconcurrency: cancel-in-progress: truegroup.isFallbackDispatchInFlight/markFallbackDispatched/clearFallbackDispatchMarker), written synchronously right after a successful dispatch (no API round-trip to race against), cleared by theworkflow_runwebhook handler on ANY conclusion (success/failure/cancelled/timed_out — not just success, so a genuinely failed run doesn't block a retry for the rest of the window), and self-expiring after the workflow's 15-minutetimeout-minutes(+ a queueing buffer) as a fail-safe if a webhook delivery is ever lost.try/catchrather than.catch()on the R2 call's return value — a minimal/partialR2Bucketimplementation that doesn't implement a given method throws synchronously at the call site, before.catch()would even attach. Caught this via a test-mock gap while writing regression tests, not from a production report.Scope
CONTRIBUTING.md.Validation
git diff --checknpm run typechecktest/unit/actions-fallback.test.ts,test/unit/actions-fallback-webhook.test.ts,test/unit/visual-capture.test.ts,test/unit/visual-config-wiring.test.ts,test/unit/focus-manifest.test.ts,test/unit/queue.test.ts— all 1411 tests pass.actions-fallback.ts100% stmts/branch/funcs/lines;capture.ts's one uncovered line (236) is pre-existing, unrelated code this PR didn't touch.npm run test:workers,build:mcp,test:mcp-pack,ui:openapi:check,ui:lint,ui:typecheck,ui:build,npm audit— not re-run locally for this exact commit (none of these files touch the OpenAPI/MCP/UI surfaces); CI runs them authoritatively.Safety
Notes
review.visual.actions_fallback), unaffected by this change either way — this only hardens the dispatch guard's own internal correctness.