github capture: hasSavedWork counts failure residue as backlog and misses budget-boundary rotations, mis-timing the retry cadence after a failed enumeration

[philcunliffe]

Source: PR #1302, triaged at head cdf51d2a5517e3214cf2e60cd8a41bcc95a742c4 (review round 2 residual finding 4).
Severity: MEDIUM (accepted, not fixed in the PR).

Location

• hypaware-core/plugins-workspace/github/src/capture.js:745 (hasSavedWork), used at capture.js:114 on the failed-enumeration early return

Observed evidence and behavior

When all_visible enumeration fails, the tick reports pending: hasSavedWork(cursors), which counts any repository cursor holding work. That set is approximate in both directions:

• Over-report: a repository that keeps failing leaves work behind, and the per-repo catch (capture.js:162) deliberately refuses to call that backlog (LLP 0360#cadence). While enumeration is also failing, nextCaptureDelay (source.js:148) clamps a 24h source to the 15-minute backlog cadence for as long as both faults persist: one extra failing listViewerRepos call per 15 minutes against an already-broken endpoint.
• Under-report: a rotation that exhausted its budget exactly at a repository boundary marks pending only via visited < repos.length (capture.js:182), which no cursor records. A following failed enumeration then reads false and the saved rotation resumes a full poll interval (up to 24h) later instead of in 15 minutes.

The PR corrected the JSDoc claim (the doc now states the approximation and both directions) instead of the behavior.

Why deferral is safe

Both residual errors are bounded and non-destructive: the worst case is a mis-timed retry or one delayed resume, never lost rows or a corrupted cursor. Both self-heal the moment enumeration succeeds, because the success path recomputes pending from the real signal. The pathological combinations also require enumeration to be persistently broken, which hyp status surfaces via lastError. An exact answer is not on disk today: distinguishing budget residue from failure residue needs a new cursor field, and the alternative (pending: undefined meaning "this tick learned nothing", so source.js keeps its previous flag, matching what the old throw path did) ripples through the shared return type in tick.js, commands.js, and source.js.

Acceptance condition

A failed enumeration leaves the source's backlog cadence exactly as accurate as the durable state allows in both directions: failure residue alone does not pin a daily source to the 15-minute cadence, and a budget-exhausted rotation interrupted by a failed enumeration still resumes on the backlog cadence. Tests cover both: (a) cursors holding only failure-residue work plus a failed enumeration do not yield the backlog cadence, and (b) an exhausted-at-boundary rotation followed by a failed enumeration does. (A pending: undefined "learned nothing" contract or an explicit durable pending marker both satisfy this.)

[philcunliffe]

Stuck: the defect this issue describes does not exist on master. Its subject is code that lives only on PR #1302's unmerged branch, so no test can fail on master and then pass.

What I did

Worked from a detached worktree at origin/master (57438f44), as instructed: base on master, read #1302 for context, do not fold its changes in.

Why the reproducing tests could not be written

hasSavedWork does not exist on master. Neither does the failed-enumeration early return at capture.js:114 that the issue's Location section names:

$ grep -rn "hasSavedWork\|inventory_resolve_failed" --include=*.js .   # nothing

On master, captureRepos calls resolveRepos unguarded (capture.js:88). A failed all_visible enumeration therefore throws straight out of captureRepos and runCaptureTick, and startGithubSource's whole-tick catch (source.js:60) records lastError while leaving backlogPending untouched.

That is precisely the "pending: undefined, this tick learned nothing, keep the previous flag" contract the issue names as option 2. master already implements it, by the throw. The pending: hasSavedWork(cursors) approximation that over- and under-reports is introduced by PR #1302 and exists nowhere else.

Evidence: both acceptance conditions already hold on unmodified master

I wrote acceptance (a) and (b) as end-to-end source-level tests, driving the real startGithubSource against a fake client whose listViewerRepos succeeds once and fails afterwards, then asserting status().details.backlog_pending. Both pass on master with no code change, and both were checked to be non-vacuous by dumping the persisted github-cursors.json:

• (a) failure residue only, then a failed enumeration. One repository whose listPullRequestsPage always throws, leaving durable residue. Cursors after the run: {"owner/a": {"pull_numbers": [], "work": {"mode":"poll","phase":"pulls","pull_tasks":[]}}} - work is present, so hasSavedWork would return true (the over-report). Observed on master: backlog_pending: false. Correct, and consistent with LLP 0360#cadence ("failures retry on the ordinary cadence").
• (b) budget exhausted exactly at a repository boundary, then a failed enumeration. Two empty repositories with captureRequestLimit: 4, so owner/a completes on the last request and the loop breaks at owner/b with pending = true via visited < repos.length. Cursors after the run: {"owner/a": {"pull_numbers": []}}, "next_repo": "owner/b" - no cursor holds work, so hasSavedWork would return false (the under-report). Observed on master: backlog_pending: true. Correct.

So on master the source's backlog cadence is already exact in both directions, and the acceptance condition is met before any change. Landing either contract option here would be a no-op refactor of code that is not present, defended by tests that never failed - which the ground-truth gate (LLP 0002) forbids.

The concrete question a human must answer

The finding is real, but it is a property of PR #1302's diff, not of master. Pick one:

1. Fold the fix into PR Fix #1298: github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1302. github capture: hasSavedWork counts failure residue as backlog and misses budget-boundary rotations, mis-timing the retry cadence after a failed enumeration #1305 becomes a review comment on that PR rather than an independent issue: when Fix #1298: github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1302's catch lands, it should return the "learned nothing" contract (propagate pending: undefined through tick.js/commands.js and have source.js keep its previous flag) instead of pending: hasSavedWork(cursors). That preserves exactly the behavior master has today via the throw, needs no new cursor field, and makes github capture: hasSavedWork counts failure residue as backlog and misses budget-boundary rotations, mis-timing the retry cadence after a failed enumeration #1305 zero net lines once Fix #1298: github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1302 is corrected. PR Fix #1298: github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1302 is currently neutral:stuck, so this needs a decision on that PR first.
2. Close github capture: hasSavedWork counts failure residue as backlog and misses budget-boundary rotations, mis-timing the retry cadence after a failed enumeration #1305 as not-applicable-to-master, and re-file it against Fix #1298: github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1302's branch if and when that PR is revived.
3. Something else - for instance, if you want the two acceptance tests landed on master as characterization tests that pin the current correct behavior before Fix #1298: github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1302 changes it. Say so and I will open that PR, but note they pass today, so they are a guard, not a fix.

My recommendation is (1), with the undefined contract rather than a new durable cursor field: master demonstrates that contract is already the right answer, and the repo rule against inventing schema fields points the same way.

Reply with a comment on this issue; neutral monitors this thread and will re-engage with your guidance on its next tick.