Skip to content

fix(control-plane): make the hook redelivery sweep cover the window it claims - #1071

Merged
zfy0701 merged 2 commits into
mainfrom
fix/hook-redelivery-coverage
Aug 16, 2026
Merged

fix(control-plane): make the hook redelivery sweep cover the window it claims#1071
zfy0701 merged 2 commits into
mainfrom
fix/hook-redelivery-coverage

Conversation

@zfy0701

@zfy0701 zfy0701 commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

HookRedeliveryReconciler is the recovery path for a GitHub delivery that never produced a HookRun — GitHub itself never retries. Three things kept it from reaching one:

  1. One page. It read the newest 100 deliveries. On an App whose repositories run CI, that is a few minutes of traffic against a 30-minute look-back window; anything older was simply invisible. The existing code warned "delivery page full — the look-back window may be under-covered", which is exactly what a busy install logs on every sweep.
  2. Coverage claimed anyway. After that truncated read it set coveredUntil = newest, declaring the whole window swept. The unlisted slice was never revisited — the one delivery the job exists to recover stays lost.
  3. First sweep a full interval after boot. A deployment that restarts the CP more often than intervalMs (10 min) never sweeps at all, and the window most worth recovering is precisely the outage that preceded the restart.

Fix

  • githubRequestPage surfaces the rel="next" cursor (githubRequest keeps its shape, delegating). A next that does not stay under the same API base is dropped rather than followed.
  • listHookDeliveries walks that cursor until a page reaches past deliveredSince, bounded by a page budget (10), and returns { deliveries, truncated }. truncated is true only when the budget ran out with a cursor still in hand — so "short because the App is quiet" and "short because we stopped" stay distinguishable, which the row count alone cannot do.
  • The sweep requests its whole look-back window, and on a truncated listing advances coverage only to the oldest delivery actually listed, so the next sweep resumes exactly where this one stopped. A complete listing advances to the ceiling as before. The warning now says what was reached instead of that a page was full.
  • The first sweep of a process runs 60s after boot; the cadence is unchanged after that.

Test plan

  • github.test.ts (new): the cursor walk stops at the floor and concatenates pages newest-first; the page budget reports truncated; a next pointing off the API base is never followed.
  • hookRedeliveryReconciler.test.ts (new): the sweep asks for the whole window; a truncated listing makes the next sweep resume at its own floor; a short-but-complete listing still advances coverage; the first sweep fires 60s after start(), not an interval later.
  • pnpm --filter @agentconnect.md/control-plane test:unit (1712 passed), typecheck, eslint, prettier.

Follow-up (deliberately not here)

coveredUntil is still per-process memory: a restart re-anchors it to now − windowMs. Persisting it would close the last gap for a CP that restarts inside an outage; it needs a schema migration and is worth its own change.

🤖 Generated with Claude Code

…t claims

The redelivery reconciler is the recovery path for a delivery that never
produced a HookRun, but three things kept it from reaching one: it read a
single 100-item delivery page (minutes of traffic on a busy App, against a
30-minute window), it then advanced its coverage cursor to the window ceiling
regardless — declaring a slice swept that was never listed — and its first
sweep came a full interval after boot, so a process that restarts more often
than that never swept at all.

- `listHookDeliveries` walks the `rel="next"` cursor to `deliveredSince`,
  bounded by a page budget, and reports whether it got there. `nextPath` is
  only followed when it stays under the same API base.
- The sweep asks for its whole look-back window and, on a truncated listing,
  advances coverage only to the oldest delivery actually listed — so the next
  sweep resumes exactly where this one stopped instead of sliding past the
  unlisted slice. A short-but-complete listing (a quiet App) still advances
  normally: the distinction comes from the walk, not from the row count.
- The first sweep of a process runs after 60s, not a full interval.

Tests: cursor walk stops at the floor, truncation is reported and never
followed off-base; the sweep requests the whole window, a truncated listing
resumes at its own floor while a complete one advances, and the first sweep
fires early.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one blocking correctness regression in the new truncated-coverage path. On a sufficiently busy App, the page budget can be exhausted entirely inside the configured two-minute grace period. The reconciler then advances coveredUntilMs beyond the ceiling it actually evaluated, causing those deliveries to be skipped when they later become eligible. The inline comment describes the required bound.

I verified the local checkout is the trusted synthetic merge with parents 52e7754a2e672a4c464709e27d86af10f8b8bcf0 and fdf2812bfc1bfb7c282a14ea8e172890f52960ee, and inspected the exact PR patch plus surrounding configuration/callers. I could not run the focused Vitest suite because this isolated checkout has no installed dependencies.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

Comment thread packages/control-plane/src/orchestrator/hookRedeliveryReconciler.ts Outdated
…ted ceiling

A page budget can be exhausted entirely inside the grace window, leaving every
listed delivery newer than `newest` — which the candidate filter deliberately
skips. Advancing coverage to that listing floor would carry the cursor PAST
deliveries the sweep withheld, and the next tick would then reject them as too
old: a new loss window on exactly the high-volume Apps pagination is for.

Coverage now stops at whichever comes first, the listing floor or the ceiling
this sweep actually evaluated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prior blocking grace-window regression is fixed: truncated coverage is now capped at the sweep’s newest ceiling, so deliveries intentionally withheld by the grace period remain eligible on the next sweep. The added test covers that high-volume case, and I found no remaining blocking regressions in the updated patch.

I verified the checkout is exactly head 2f0de05733b7680cd9b31cd868f67936cbd22491 and reviewed both the synchronization delta and the complete pagination/reconciler flow. I could not rerun Vitest locally because this isolated checkout has no installed dependencies.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

@zfy0701
zfy0701 merged commit 730fe84 into main Aug 16, 2026
11 checks passed
@zfy0701
zfy0701 deleted the fix/hook-redelivery-coverage branch August 16, 2026 03:31
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