fix(publication): stop repository pool on first throttle - #1145
fix(publication): stop repository pool on first throttle#1145brokemac79 wants to merge 8 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs maintainer review before merge. Reviewed August 14, 2026, 7:50 AM ET / 11:50 UTC. ClawSweeper reviewWhat this changesThis PR adds a default-disabled shared coordinator that stops repository-token publication egress after a GitHub throttle and recovers it through bounded probe cohorts. Merge readinessKeep open for maintainer review: the coordinator is not implemented on current main, but this draft is dirty against substantial newer publication work and needs a rebase plus fresh merge-result proof before landing. Priority: P2 Review scores
Verification
How this fits togetherExact-review publication sends review results and lifecycle actions through GitHub credential pools. The proposed coordinator sits before repository-token egress and returns deferred work to the durable queue when that shared pool is throttled. flowchart LR
A[Queued exact reviews] --> B[Publication runner]
B --> C[Repository credential pool]
C --> D{Throttle observed?}
D -->|No| E[GitHub publication]
D -->|Yes| F[Coordinator deferral]
F --> G[Durable queue retry]
E --> H[Lifecycle completion]
Decision needed
Why: Current main changed adjacent publication retry behavior after the PR base, and the current merge state is dirty, so accepting the combined runtime behavior requires maintainer approval. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: If maintainers want the shared pool fence, rebase it onto current main, retain the current unattempted completion contract, and attach fresh exact-head Crabbox proof for the merged workflow, queue, and runtime paths. Do we have a high-confidence way to reproduce the issue? Not applicable as a feature PR; the contributor provides a detailed controlled runtime scenario, but it has not been rerun against the current-main merge result. Is this the best way to solve the issue? Unclear: the shared fence is a coherent design, but rebasing and proving its interaction with the newer publication path is the safer solution before merge. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e17e09425b60. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
aa63d48 to
3765859
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
3765859 to
6f78f96
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Summary
Add a default-disabled interlock that stops
repository_actionsexact-review publication egress on the first GitHub throttle observed for that credential pool.The interlock is a pool-sharded SQLite Durable Object. Each external operation acquires and starts a short fenced permit. A 403/429 advances the shared epoch synchronously, so already-claimed siblings that have not started are rejected before wire and durably requeued as
attempted=false. Calls already on wire may finish and are counted. Recovery preserves authoritative reset headers, admits exactly one half-open probe, and ramps through fixed1 -> 2 -> 4 -> 8cohorts.Enforcement remains off by default. This PR neither activates the coordinator nor changes scheduled admission (
300/hour, burst30), hot-review cadence, queue contents, credentials, gates, or production state.Problem
Phase 0 telemetry established that publication work can claim up to eight sibling batches before the first batch observes a shared GitHub throttle. Without a pool-wide fence, siblings learn about the unavailable credential independently, creating avoidable requests and retry amplification.
The constrained resource is the credential pool, not an item or target repository. The required invariant is: after the first authoritative throttle observation, no not-yet-started operation using that pool may reach GitHub; already-on-wire work remains bounded and completes under existing mutation and idempotency rules.
This is deliberately not a fleet-wide quota cure. Phase 0 measured
public_read_fallbackandtarget_appas separate cohorts. This PR coordinates only the correctedrepository_actionspublication boundary and never infers a pool from generic rate-limit text.Implementation
GithubEgressPoolCoordinator, a SQLite-backed Durable Object keyed by a server-derived private credential-pool identity. It is not keyed by item/repository and is not one global singleton.ghinvocation, with separate acquire/start/finish/throttle transitions and epoch fencing.permits_in_flight_at_openandalready_on_wire_completions.Retry-AfterandX-RateLimit-Resetauthority. Missing or non-credible deadlines use one persisted bounded exponential backoff with deterministic jitter rather than independent 60-second reopen loops.1,2,4, and8. Cohorts cannot refill after partial completion, cannot advance with unresolved work, and reopen on failure or expiry.attempted=false, without consuming publication failure, retry, DLQ, or mutation budgets. Genuinely attempted failures retain existing accounting.gh api, lifecycle/comment routers, artifact download, repository-token workflow dispatch, terminal confirmation, and repository-token reads in apply-decisions. Target-App mutations remain outside this coordinator, so the change does not transfer pressure to App installations.unexecuted_failure, the member is deferred asattempted=false, and no wire call occurs.Scope and dependencies
Base:
56cb78d60734ddc62b5f1e49981bbb4556dcb58dHead:
6f78f960950c822c923f7cf7f94a5211ce20f95bThe base includes Phase 0.5 PR #1150. Phase 0.5 owns signed observational reset propagation into durable queue credential circuits, deterministic per-member recovery jitter, and objective handoff recovery reasons. This PR consumes those current-main contracts without duplicating or weakening them. The coordinator remains independently reviewable and default-disabled; its later activation limits require a separate maintainer decision and stable production evidence.
Observed Phase 0.5 recovery is calibration/design evidence only: an authoritative
repository_actionshold stopped dispatch through reset plus bounded jitter, then resumed service and drained backlog. It is not evidence that this Phase 1 coordinator has been activated or safely calibrated in production.The PR remains independent of #1110's adaptive hot allocator and does not copy its scheduler observations or cursor machinery. It also excludes generic batching, request elimination/cache work, and the broader Cloudflare architecture.
Real Behavior Proof
Claim
Eight publication siblings share one
repository_actionspool. Three reach the started boundary before the first 403. That throttle advances the shared epoch; five acquired-but-unstarted siblings never reach the loopback GitHub server, and the two other already-on-wire calls are the only bounded completions. State persists across Worker restart. At reset, one of eight contenders receives the probe; successful recovery proceeds through fixed1 -> 2 -> 4 -> 8cohorts. Target-App work remains independent.Exercised surface
6f78f960950c822c923f7cf7f94a5211ce20f95bfrom a checksum-pinned Git bundle;attempted=false, pool independence, rollback, and privacy sentinels.Environment and command
Docker-backed Crabbox
local-container:local-container;cbx_8e8581647ea2;0dd7aca21129;clawsweeper-proof:node24-jq(Node 24.19.0);bash docs/proof/github-egress-pool-coordinator/run-proof.shwithGITHUB_EGRESS_POOL_PROOF_SOURCE_SHA=6f78f960950c822c923f7cf7f94a5211ce20f95b;2616a39c7f845fae89e36d64c9eeb70976062458662e8728eabd8f00790d056c;4b112df68f60c2e124daf6341a91458803fde1e9063d9a699aa397aacc73236a.Observed result
1 -> 2 -> 4 -> 8 -> closed, with no cohort refill while unresolved;attempted=false; one request was avoided;gh run downloadfirst throttle opened the pool; sibling performed zero loopback requests;The retained proof package contains
proof-summary.json, sanitized public coordinator state, focused-test/build/install logs, and redacted Wrangler output.Limits
ghinvocation. Opaque binary artifact transport internals are not fabricated as page counts.Validation
0.actionlint1.7.12 passed for all workflows, excluding only the repository's documentedconcurrency.queuecompatibility warning.pnpm run test:workflow-sparse-checkout: 4/4 passed.pnpm run test:no-build: 3,442 tests; 3,434 passed; 0 failed; 8 expected platform skips.git diff --check 56cb78d60734ddc62b5f1e49981bbb4556dcb58d..6f78f960950c822c923f7cf7f94a5211ce20f95b: passed; worktree clean.codex review --base origin/main: no actionable correctness issue.Risks, rollout, and rollback
The merge/deploy state is dark.
CLAWSWEEPER_REPOSITORY_POOL_COORDINATOR_ENABLEDdefaults to false, and unsafe coordinator configuration fails validation before serving permits. No activation is authorized by this PR.Later activation must be a separate maintainer decision after Phase 0/0.5 stabilization on a fixed deployment SHA and complete quota windows. A canary must demonstrate avoided-before-start growth after throttles, zero retry/DLQ charge for coordinator deferrals, exactly one probe per reset epoch, bounded on-wire remainder, healthy other-pool progress, and no publication SLO regression.
Rollback is configuration-only: set
CLAWSWEEPER_REPOSITORY_POOL_COORDINATOR_ENABLED=false. The legacy execution path remains present, persisted coordinator state is ignored while disabled, and rollback requires no queue, DLQ, or state rewrite. Revert is not recommended for backlog alone; use rollback only for coordinator-attributable correctness, isolation, or accounting regressions.Bay impact
None while disabled. The public surface is sanitized operator telemetry and does not alter lifecycle projection. The real-boundary receipt records
openclaw_bay_affected=false.Non-goals