feat(rekor-monitor): resumable identity scan to catch up large backlogs - #1929
Conversation
lockwobr
added
area/ci
theme/supply-chain
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Rekor monitor now performs bounded, resumable identity scans using a persisted Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/rekor-monitor/checkpoint.go`:
- Around line 219-275: Extract the shared stat, archive-size validation, and
zip.OpenReader logic from extractCheckpointFromZip and extractProgressFromZip
into a helper that opens and validates the archive once. Update both extraction
functions and restore to reuse this helper appropriately, centralizing the
maxArchiveBytes enforcement while preserving their existing entry-selection and
error behavior.
In `@tools/rekor-monitor/main.go`:
- Around line 197-207: Update the progress restoration logic around readProgress
so a persisted progress equal to end is accepted and scanFrom is set to end,
preventing a completed window from being rescanned. Preserve the existing
start-boundary behavior and add a regression test that seeds writeProgress(end),
calls observe(), and verifies checkpoint advancement without rescanning.
In `@tools/rekor-monitor/observe_test.go`:
- Around line 236-329: Add a regression case near TestObserveIncrementalCatchUp
that seeds persisted progress at the current scan window’s end before calling
observe. Assert the checkpoint advances to the monitor head, progress resets,
and fakeMonitor.scanRanges shows no full-window rescan (ideally no scans),
preserving the existing behavior for normal incremental catch-up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c89654f4-ea79-497c-8524-3ee1130a9796
📒 Files selected for processing (8)
.github/workflows/rekor-monitor.yamldocs/contributor/maintaining.mdtools/rekor-monitor/checkpoint.gotools/rekor-monitor/checkpoint_test.gotools/rekor-monitor/doc.gotools/rekor-monitor/main.gotools/rekor-monitor/monitor.gotools/rekor-monitor/observe_test.go
Coverage Report ✅
Coverage BadgeMerging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
3aab8f9 to
e7403a7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
e7403a7 to
72fd124
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributor/maintaining.md`:
- Around line 195-197: Update the closing sentence in the maintaining
documentation to use the clearer “This is what un-wedged…” construction, retain
the 2026-07-24 backlog size and issue link, and connect it with the explanation
that the earlier single-pass scan timed out on every run.
In `@tools/rekor-monitor/checkpoint.go`:
- Around line 136-143: Update checkpointStore.writeProgress to create the
directory returned by s.progressPath() before calling os.WriteFile, using the
same directory-creation behavior as extractCheckpointFromZip and store.write;
preserve the existing file permissions and error wrapping.
In `@tools/rekor-monitor/main.go`:
- Around line 205-216: The resume-at-end branch in the checkpoint scanning flow
must make the skipped window visible in its report. Before calling out.report(w)
when progress >= end, populate out.scanned, out.from, out.to, and out.caughtUp
consistently with a completed scan, or emit an equivalent one-line recovery
note, while preserving the existing checkpoint advancement.
In `@tools/rekor-monitor/monitor.go`:
- Around line 207-231: Document the concurrency constraint for the test-mutable
scan bounds in the var block or the withScanBounds helper: tests that reassign
scanChunkSize, maxScanEntriesPerRun, or scanBudgetHeadroom must not use
t.Parallel(), since concurrent mutation is unsafe and can trigger race reports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6b0e1390-bd48-447c-8e68-1f9f9ac31fca
📒 Files selected for processing (8)
.github/workflows/rekor-monitor.yamldocs/contributor/maintaining.mdtools/rekor-monitor/checkpoint.gotools/rekor-monitor/checkpoint_test.gotools/rekor-monitor/doc.gotools/rekor-monitor/main.gotools/rekor-monitor/monitor.gotools/rekor-monitor/observe_test.go
63e6081 to
ecca0d3
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
📋 Multi-persona review
▎ Method: 4 independent persona passes (Correctness & State Machine, Security & Supply Chain, Operability/CI-DX, Test-coverage & Domain), with every finding then confirmed, refuted, or re-tiered by a senior meta-reviewer against the resolved code. Line links pinned to head e7403a7b.
▎ Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick · ✅ Confirmed non-issue
Overall assessment — Approve with comments
This is a well-constructed fix for a real wedge, and the design instinct — splitting the consistency anchor (signed checkpoint) from the scan cursor — is the right one. The core safety property holds: I tried five separate angles to construct a case where a partial-clean pass lets a real finding slip past, and could not (see non-issues below). TestObserveIncrementalCatchUp is genuinely rigorous — three real observe() calls with a chunk walk that fails on both a gap and an overlap, which is exactly the property that matters here. Lint is clean and coverage is up ~1.1% over main.
Nothing blocks merge. The one thing worth fixing before merging is a pair: advanceCheckpoint's write ordering and the progress > end fail-open. Individually each is narrow; together they form the only path to a silent identity-scan coverage gap that isn't already gated by the artifact-provenance filter. Both fixes are a few lines.
Three findings that GitHub wouldn't let me anchor inline
(Two land on unchanged context; the third is in a region the PR diff doesn't expose.)
🔵 Nitpick — redundant o.scanned && inside a branch already guarded by o.scanned (tools/rekor-monitor/monitor.go:301)
report returns early at L297-299 (if !o.scanned { return }), so o.scanned is unconditionally true by the time you reach if o.scanned && !o.caughtUp { and the conjunct is dead. Suggest if !o.caughtUp {.
🔵 Nitpick — scanIdentity's doc comment says [start, end] where the API is exclusive-start (tools/rekor-monitor/monitor.go:122)
The comment reads "searches the [start, end] entry-index window", which reads inclusive, while scanWindow (L239-244) correctly documents the exclusive-start (start, end] semantics of tiles.GetEntriesByIndexRange. Exclusive-start is the correct reading — scanWindow passes prev.Size-1 precisely to include index prev.Size.
The comment is pre-existing and untouched by this PR, but the PR makes it load-bearing in a new way: the chunk loop's gap-free property depends on reached = chunkEnd making one chunk's end the next chunk's exclusive start. A maintainer trusting the inclusive comment would "fix" that to reached = chunkEnd + 1, silently skipping one entry per chunk boundary (6 per 300k run). Suggest: // scanIdentity searches the (start, end] entry-index window (exclusive of start; see scanWindow) ...
🔵 Nitpick — selectCheckpointEntry's sole-entry fallback can now select the .scan companion (tools/rekor-monitor/checkpoint.go:223)
An archive containing only checkpoint_v2.txt.scan has no base-name match for checkpoint_v2.txt, so if len(files) == 1 { return files[0], nil } returns the companion and its integer body is written to the checkpoint path.
This fails closed — ReadLatestCheckpointRekorV2 can't parse 129, so it returns a wrapped ErrCodeInternal → operational, cursor unchanged. It's also unreachable today (.scan can't exist without the checkpoint, given the write ordering). The residual is that the fallback's safety now rests on an invariant enforced in another file rather than on the archive shape, and it's inconsistent with extractProgressFromZip, which deliberately declines the same fallback and says so. Consider dropping the fallback now that the artifact reliably contains a correctly-named member.
✅ Confirmed non-issues (checked and cleared)
Listed so you know these were examined, not missed — several were raised by a persona and then refuted on closer reading:
- The central safety claim holds. A finding returns before
writeProgress, andrunEndis anchored toscanFrom, so the finding chunk is always the first chunk from the persisted boundary and the per-run cap can never step over it. Tested against changingknownTags, log growth, and the retry path. The only counterexample is shard rotation (filed inline). - Closing a tamper alert on a partial pass is correct —
checkConsistencyis not partial. It runs in full every pass and is a precondition for reaching the scan loop, so a capped pass has genuinely proved the log append-only fromprevtocur. - Closing the degraded issue on a partial pass is defensible — its stated condition is "unable to complete" for 3+ runs, and a capped pass did complete within budget and made forward progress. Only the "completed cleanly" wording overstates coverage (folded into the catch-up-observability comment).
actions/upload-artifactmulti-path is fine — both paths resolve under$GITHUB_WORKSPACE, so the least-common-ancestor is the workspace root and members stay barecheckpoint_v2.txt/checkpoint_v2.txt.scan;filepath.Basematching in both extractors works unchanged.overwrite:is not needed (v4+ only rejects duplicate names within the same run, and there's one upload step).- Stale progress is always ≤ the new
startfor a same-shard advance, soprogress > startis false and the scan safely restarts atstart— no silent skip. (This is not true across a shard rotation, which is why theadvanceCheckpointordering comment exists.) - Timeout composition is sound — one 45m context bounds all three attempts and every network call within them, and fires before the 90m job timeout, leaving the notification steps room.
- No new template-injection surface — every
run:block uses plain$VAR/${VAR}sourced fromenv:; the added expressions are confined toif:conditions and the upload step'swith:/path:, neither shell-interpolated. maxScanEntriesPerRundoes not compound across retries. A worry that 3 attempts could scan 3×300k is backwards: retries only fire on operational failures, and each retry re-restores the older progress, so extra attempts repeat work rather than extend coverage.- Quality gates —
golangci-lint run -c .golangci.yaml ./tools/rekor-monitor/...→ 0 issues;go vetclean; suite passes with-race. Coverage 82.8% at head vs 81.7% onmain(+1.1%, no regression — the PR body's 82.1% slightly understates it). No new function at 0%; the 0% entries are all pre-existing network-backed code.
Summary
| Tier | Count | Items |
|---|---|---|
| 🔴 Blocker | 0 | — |
| 🟠 Major | 0 | — |
| 🟡 Minor | 5 | advanceCheckpoint ordering · progress > end fail-open · silent catch-up / no stall alert · retry discards progress · rotation drops backlog |
| 🔵 Nitpick | 8 | workflow comment · doc tense + absolutes · throughput arithmetic · dead conjunct · single-run finding test · chunk-size guard · scanIdentity comment · zip fallback |
Recommendation: Approve with comments. Fixing the 🟡 pair (advanceCheckpoint ordering + progress > end) together before merge would close the only silent-coverage-gap path — a few lines each. The other three 🟡 are reasonable to defer in writing; the catch-up observability one is worth a follow-up issue, since a green-but-not-converging monitor is a real loudness regression versus the wedged state it replaces.
Thanks for the unusually thorough comments and rationale in this change — the SECURITY: block on the artifact fetch and the sizing rationale on the bounds vars made this much faster to review than it would otherwise have been.
| if !out.caughtUp { | ||
| out.remaining = end - reached | ||
| } | ||
| out.report(w) |
There was a problem hiding this comment.
🟡 Minor — advanceCheckpoint writes the checkpoint before resetting progress, so a failed reset uploads a mismatched pair
The two files are non-atomic, and the Persist step uploads whatever is on disk (if: !cancelled()). If store.write succeeds and store.writeProgress(0) then fails, the artifact pairs a new checkpoint with stale progress.
On the caught-up path that's harmless — the stale value coincidentally equals the new start. The dangerous path is shard rotation (L182): it advances to a new shard whose Size is small, while the stale progress is a six- or seven-figure old-shard index. The next run then either short-circuits via progress >= end (L205) or jumps scanFrom forward (L214), skipping entries with the cursor advanced past them.
Worth noting on the trigger: a runner OOM/eviction kills the job so nothing uploads, and a job-timeout kill is a cancellation which !cancelled() skips — so the realistic trigger is a writeProgress I/O failure, not a crash. Narrow, but it is the only way to desynchronise the two files.
Blast radius: Up to a full window of unscanned entries with the cursor advanced past them, reported clean. This is the finding I'd most want addressed before merge — paired with the progress > end comment below, it forms the only path to a silent coverage gap that isn't already gated by the artifact-provenance filter.
Fix: Swap the order — strictly safer, since this is only reached when the window is fully scanned (or has nothing to scan), so a failure at any point leaves (old checkpoint, progress 0) = a harmless full re-scan:
func advanceCheckpoint(store checkpointStore, prev, cur *tlog.Checkpoint) error {
if err := store.writeProgress(0); err != nil {
return err
}
return store.write(prev, cur)
}There was a problem hiding this comment.
Fixed in 86e02d97: advanceCheckpoint now resets progress first, then writes the checkpoint, so a failed second write leaves the safe (old checkpoint, progress 0) pair rather than a new checkpoint over stale progress. Good catch.
| if err != nil { | ||
| return err | ||
| } | ||
| if progress >= end { |
There was a problem hiding this comment.
🟡 Minor — progress >= end advances the signed checkpoint on a state the code itself calls impossible
The comment on L210 asserts "the log is append-only so progress can never legitimately exceed end" — and the code then handles that impossible state by advancing the cursor with zero entries scanned, reporting clean. readProgress validates only parseability and n >= 0. CLAUDE.md names fail-open-on-an-ambiguous-condition as an anti-pattern.
For the record on why this is tiered Minor rather than a security finding: the .scan companion travels in the same artifact zip as the checkpoint, so anyone who can plant one can plant the other — and a poisoned genuine head checkpoint is a strictly stronger lever (it collapses the window to empty and still satisfies the Merkle proof), which the head_repository_id/head_branch filter already stops. Corruption also can't reach the bad direction: truncating a decimal only yields a smaller value (absorbed by progress > start), and non-numeric garbage hard-errors at checkpoint.go:127.
Blast radius: One window's entries never identity-scanned, reported clean, cursor advanced past them irrecoverably. progress == end is the legitimate interrupted-advance case (covered by TestObserveResumeAtWindowEnd) and must keep advancing; only > end is the hole.
Fix: Split the branch, and fix it together with the advanceCheckpoint ordering above — that is what makes > end reachable without corruption:
if progress == end { out.report(w); return advanceCheckpoint(store, prev, cur) }
if progress > end {
return errors.New(errors.ErrCodeInternal, "scan progress exceeds window end; refusing to advance")
}There was a problem hiding this comment.
Fixed in 86e02d97: split into == end (advance — the legitimate interrupted-advance case) and > end (fail closed with an error, refuse to advance). Added TestObserveProgressBeyondWindowFailsClosed.
| reached = chunkEnd | ||
| if perr := store.writeProgress(reached); perr != nil { | ||
| return perr | ||
| } |
There was a problem hiding this comment.
🟡 Minor — Catch-up runs are indistinguishable from full coverage; a stalled scan never alerts
A capped pass returns nil → exit 0 → CLASSIFICATION=clean. The workflow's monitor step greps only ^CLASSIFICATION=, so nothing consumes the catching up, N entr(y/ies) remaining line, and nothing tracks progress run-over-run. A permanently non-converging catch-up therefore reports healthy hourly, indefinitely.
Convergence is real but is an unguarded assumption in a comment (300k/run vs ~70k/hr, monitor.go:216-221). If growth ever exceeds the cap — or progress keeps getting re-lost — it diverges silently.
Blast radius: This is a genuine regression in failure loudness: pre-PR an oversized window timed out, classified operational, and opened a degraded issue after three runs. Post-PR the same condition is green. Tiered Minor because divergence needs a >4x growth change, and the converging behavior strictly improves on the wedged status quo.
Fix: Cheapest: emit a distinct marker on the partial path (e.g. CATCHUP_REMAINING=<n>) alongside CLASSIFICATION=, gate the two gh issue close blocks on its absence, and fix the "completed cleanly" close-comment wording. Stronger: persist remaining alongside progress and return an operational error once it fails to decrease across N consecutive runs, so the existing 3-strike degraded path fires on its own.
There was a problem hiding this comment.
Implemented the stronger option in 86e02d97: a <checkpoint>.stall companion tracks remaining across runs; if it fails to decrease for 3 consecutive passes, observe returns a new degraded classification (distinct from operational, so it is not retried) that feeds the existing 3-strike degraded-issue path. It auto-recovers to clean once catch-up resumes. Tests: TestObserveCatchUpDivergesGoesDegraded and ...ConvergesStaysClean.
| if capped := scanFrom + maxScanEntriesPerRun; capped < runEnd { | ||
| runEnd = capped | ||
| } | ||
| reached := scanFrom |
There was a problem hiding this comment.
🟡 Minor — The retry loop re-restores the artifact, discarding the failed attempt's persisted progress
This comment says "progress up to reached is already persisted", and runWithRetry's doc (L373-375) says "a failed attempt leaves no partial state" — both are now inaccurate. Each retry calls run() → store.restore() → extractProgressFromZip, which ends in a bare os.WriteFile (checkpoint.go:273) with no comparison against disk, reverting .scan to the artifact's value.
Blast radius: To be clear about severity: this is work repetition, not a skip — restoring older progress only re-scans, never skips, and it reverts the checkpoint in lockstep so on-disk state stays internally consistent. The cost is throughput: at the stated rate, three attempts is ~35 min of a 45 min budget, so a single transient blip late in a slice can consume most of the pass and end operational.
Fix: Hoist store.restore(ctx) out of run() into realMain before runWithRetry (this also removes a redundant repeated zip open), or have extractProgressFromZip keep max(on-disk, archived). Update both stale comments either way.
There was a problem hiding this comment.
Fixed in 86e02d97: hoisted store.restore into realMain before the retry loop, so an operational retry resumes from the failed attempt saved progress instead of reverting to the artifact value. Updated both stale comments; the restore-error test moved to TestRealMainPropagatesRestoreError.
| // different logs, so a size-based window is meaningless and the vendored | ||
| // IdentitySearch only reads the latest shard. Re-baseline on the new shard | ||
| // (resetting scan progress) and report the gap rather than silently skipping. | ||
| if prev != nil && cur != nil && prev.Origin != cur.Origin { |
There was a problem hiding this comment.
🟡 Minor — Shard rotation discards an arbitrarily large unscanned backlog and closes an open finding alert
The rotation branch sits before store.readProgress() (L201), calls advanceCheckpoint (which resets progress to 0), and returns nil → exit 0 → the success()-gated close step closes any open alert issue.
Pre-PR the operator message at monitor.go:291-293 ("Entries around the rotation boundary are not identity-scanned") was accurate — the window was at most one hour of entries. Post-PR the cursor can legitimately sit hundreds of thousands of entries behind head mid-catch-up, so the same sentence understates the gap by orders of magnitude and gives no count.
Blast radius: Up to the entire held window (potentially ~1M entries) unscanned at the yearly rotation boundary, plus loss of an open finding alert that was holding the cursor. This is also the one counterexample I found to the PR's central safety claim that a partial-clean pass never coexists with an open finding alert.
Fix: Read progress before the rotation branch and include the abandoned count in the report line at monitor.go:291 ("N entr(y/ies) in the prior shard window were not identity-scanned"). Stronger: if progress is non-zero at rotation time, return an operational error so the run stays red and the alert survives triage.
There was a problem hiding this comment.
Partially addressed in 86e02d97. The silent-drop half is fixed: observe reads progress before the rotation branch and logs a loud WARNING with the abandoned prior-shard index (TestObserveShardRotationReportsAbandonedProgress). On the held-finding-alert half: I kept the rotation advancing (re-baseline), because observe cannot distinguish a held finding from a normal mid-catch-up at the rotation boundary without wedging legitimate rotations red forever, so I chose the loud report over a fail-closed that would also wedge the common case. The general permanently-behind case is now covered by the degraded detector above. Happy to switch to the fail-closed variant if you would rather accept the wedge for the yearly-rotation edge.
| # the tool advances the checkpoint only once the identity scan has | ||
| # caught up to head, and persists partial progress in the companion so a | ||
| # large backlog is scanned across several runs. Both must travel in the | ||
| # artifact so the next run resumes; the companion is absent on runs that |
There was a problem hiding this comment.
🔵 Nitpick — Workflow comment's two examples of "companion absent" both actually write the companion
Both cited cases fall through to advanceCheckpoint → writeProgress(0) (main.go:191-195 → 283):
- First run:
prev == nil, so the rotation branch is skipped,scanWindow(nil, cur)returnsok=false, and control reachesif !mon.watchesIdentity() || !ok. The companion is written, containing0. - Consistency-only: empty
certSubject→watchesIdentity()false → same path → companion written.
The companion is in fact absent only when the run fails before any advance or scan — which is also when the checkpoint itself is absent.
Blast radius: Comment accuracy only; if-no-files-found: ignore remains correct and necessary, just not for the stated reason.
Fix: Reword to the true condition, e.g. "both files are absent only when the run fails before it can advance the cursor (e.g. a first run whose consistency check errors), and if-no-files-found ignores that."
There was a problem hiding this comment.
Fixed in 86e02d97: reworded to the true condition. Every advance path writes the companion (advanceCheckpoint resets it), so both files are absent only when the run fails before it can advance the cursor or save a chunk.
| catch-up adapts to scan speed (a slow run covers fewer entries, a fast run more) | ||
| and never overruns the deadline; `maxScanEntriesPerRun` is only an outer safety | ||
| ceiling on a single run. The signed checkpoint advances (and the companion resets) | ||
| only once the scan reaches head. So a large backlog is caught up over several |
There was a problem hiding this comment.
🔵 Nitpick — Catch-up doc paragraph: inverted final sentence, plus two absolutes the code qualifies
The final sentence is muddled two ways: past tense asserts a completed outcome for a change that hasn't merged yet, and the colon promises the cause of the un-wedging but delivers the cause of the wedge — the clauses are inverted.
The rest fact-checks well (the quoted log wording matches monitor.go:302 verbatim; the constant names, companion name, same-artifact carriage, reset-on-advance and partial-pass-exit-0 all match). Two over-claims though: "advances only once the scan reaches head" omits three other advance paths (rotation L185, no-window/consistency-only L194, and the progress >= end short-circuit L212); and "a partial-clean pass never coexists with an open finding alert" is false on the rotation path, which returns nil and thus closes an open alert.
Blast radius: Docs only, but this paragraph is the operator's primary reference for interpreting a green-but-catching-up run, so the two absolutes could mislead about when the cursor can advance without a completed scan.
Fix: Rewrite the last sentence in present/future tense, e.g. "This is what unblocks a backlog like the ~1.2M-entry window that followed the #1902 correlation fix, where the earlier single-pass scan timed out every run and never advanced." Soften both absolutes with "(aside from a shard rotation or an empty window, which re-baseline and are reported as such)".
There was a problem hiding this comment.
Fixed in 86e02d97: present tense, and softened both absolutes. It now notes the other advance paths (baseline, empty window, rotation) and the rotation exception to the never-coexists-with-a-finding-alert claim. Added a paragraph documenting the new divergence-to-degraded behavior.
| // persisted progress cursor advances one chunk at a time. It also caps the | ||
| // overrun past the soft budget, since the budget is only checked between | ||
| // chunks (one in-flight chunk may finish after the budget is spent). | ||
| scanChunkSize int64 = 50_000 |
There was a problem hiding this comment.
🔵 Nitpick — maxScanEntriesPerRun sizing cites a throughput derived from a run that timed out
The comment says "the 2026-07-24 wedge scanned ~1.17M entries and hit the 45m deadline (~26k/min)". A run that hit the deadline by definition did not complete 1.17M in 45m — 1.17M was the window size, not completed work. So ~26k/min is an upper bound on throughput presented as an estimate, and the derived "finishes in roughly a third of the budget" is optimistic by an unknown factor.
Blast radius: Comment accuracy only, and self-limiting in practice: writeProgress fires after every 50k chunk, so a deadline hit still banks all completed chunks and the next run resumes there. The failure mode degrades to slower catch-up plus an operational classification, never a coverage skip.
Fix: Reword honestly, e.g. "the 2026-07-24 wedge had a ~1.17M-entry window and hit the 45m deadline, so throughput is at most ~26k/min; the cap is sized against that upper bound and per-chunk progress saves absorb any overrun."
There was a problem hiding this comment.
Fixed in 86e02d97: reworded to cite measured throughput from completed catch-up runs (~950k in ~40m), and it now states explicitly that the 2026-07-24 wedge only bounds the window size, not completed work.
| cur: testCheckpoint(200), | ||
| found: []identity.MonitoredIdentity{{FoundIdentityEntries: []identity.LogEntry{{CertSubject: "x"}}}}, | ||
| findAt: 135, // lands in the (129, 139] chunk | ||
| } |
There was a problem hiding this comment.
🔵 Nitpick — Finding-halt test never exercises the cross-run re-detection it exists to protect
TestObserveFindingHaltsCatchUp calls observe exactly once. It asserts the checkpoint held at 100 and progress == 129 — which is the precondition for re-detection, not re-detection itself. The property claimed at main.go:239-244 ("re-detected every run until a maintainer triages it") is cross-run and is never exercised: nothing verifies that a second run re-enters the finding chunk and re-alerts.
Credit where due — TestObserveIncrementalCatchUp is the strong one: it loops three real observe() calls and at L284-296 walks every recorded chunk asserting r[0] == wantStart, which fails on both a gap and an overlap given the exclusive-start/inclusive-end ranges, then asserts the cursor lands exactly on head-1. That's genuinely exhaustive, not endpoints-only. The finding path just didn't get the same treatment.
Blast radius: Test-coverage gap on the invariant that keeps a finding alert from auto-closing — a regression that persisted the finding chunk's progress would leave this test green.
Fix: Add a second observe(ctx, f2, store, nil, io.Discard) with a fresh fakeMonitor (same findAt, same store), asserting a non-nil finding error and that f2.scanRanges[0] starts at 129 (the finding chunk is re-entered).
There was a problem hiding this comment.
Fixed in 86e02d97: TestObserveFindingHaltsCatchUp now runs a second observe() and asserts the finding chunk (start 129) is re-entered and re-alerts, exercising the cross-run re-detection.
| // the soft time budget (scanDeadline): scan whatever fits before the pass | ||
| // deadline nears, then save progress and resume next run. runEnd is the outer | ||
| // per-run safety ceiling. After each clean chunk the progress cursor is | ||
| // persisted, so a mid-run failure, the time budget, or the per-run cap resumes |
There was a problem hiding this comment.
🔵 Nitpick — Non-positive scanChunkSize burns the whole pass deadline instead of failing fast
With scanChunkSize <= 0, chunkEnd := reached + scanChunkSize is <= reached, the clamp doesn't fire, and reached = chunkEnd leaves reached unchanged. Not an infinite spin, to be precise — ctx.Err() is the loop's first statement, so it terminates at the 45m deadline with ErrCodeTimeout → operational.
Blast radius: One wasted 45m pass; not reachable from production code, since these are package vars only so tests can shrink them. Low risk overall.
Fix: Guard before the loop: if scanChunkSize <= 0 { return errors.New(errors.ErrCodeInternal, "scanChunkSize must be positive") }. Optionally have withScanBounds take t and use t.Cleanup — note that mutating package globals forecloses t.Parallel() in this package regardless of restore mechanism (nothing calls it today, so nothing is broken).
There was a problem hiding this comment.
Fixed in 86e02d97: added a scanChunkSize <= 0 guard that fails fast (TestObserveRejectsNonPositiveChunkSize).
ecca0d3 to
20ea748
Compare
The identity scan is linear in the window size, so a window that has not advanced for a while (a multi-hour Sigstore/TUF outage, or a finding that deliberately holds the cursor) can grow past what one run can scan inside the 45m pass deadline. On 2026-07-24 the window reached ~1.17M entries and timed out (operational) every run; the monitor could not self-heal without a re-baseline, which skips the backlog (a coverage gap). Make the scan resumable. Split the signed checkpoint (the consistency anchor, advanced only once the scan reaches head) from a new scannedTo cursor persisted in a <checkpoint>.scan companion carried in the same artifact. Each run scans a bounded slice (maxScanEntriesPerRun, in scanChunkSize chunks), persists progress after each clean chunk, and resumes there next run. A large backlog is caught up over several hourly runs, each within budget, with no coverage gap and no re-baseline. A partial catch-up pass is a clean (exit 0) run; a finding still halts at its chunk and is re-detected until triaged (so a partial-clean pass never coexists with an open finding alert). The consistency proof stays O(log n), so it is never the bottleneck. Also re-enable the scheduled cron (commented out on 2026-07-24 to pause the wedged monitor) and upload the progress companion alongside the checkpoint. An artifact without the companion (a run before this shipped, or one that did not scan) resumes from 0, i.e. the prior full-window behavior. Follows #1897 / #1900 / #1903. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
20ea748 to
86e02d9
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
📋 Re-review (round 2) — Approve with comments
▎ Method: 3 focused persona passes over the new surface (correctness/state-machine, operability/workflow, test coverage), adjudicated by a senior meta-reviewer that re-derived each finding from the code and proved the test gaps by go test -overlay mutation against /tmp copies — no tracked file was modified. Pinned to head 834311d5 (round 1 reviewed e7403a7b).
▎ Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick · ✅ Confirmed non-issue
Approving. Every round-1 finding is genuinely fixed in the code, not papered over — I re-verified each: advanceCheckpoint reordered fail-closed with a correct rationale, progress > end now errors, store.restore hoisted out of the retry loop, rotation reports abandoned progress, the chunk-size guard. And you went past what I asked: replacing the entry cap with a soft time budget and adding a stall detector (.stall + catchUpStalledError + classDegraded) to close the silent-under-coverage gap is a better fix than the one I proposed. Tests pass, lint clean, coverage 82.1%.
Nothing here blocks merge. Two items are worth doing before it, both cheap; the rest is at your discretion.
🟠 Major 1 — The degraded issue tells the operator to ignore a real monitoring gap
(Not inline: the step it lands in is pre-existing text this PR now routes a new classification into.)
classDegraded exits 3 and correctly reaches the degraded-issue step, whose gate (.github/workflows/rekor-monitor.yaml:345) is the inverse of the tamper/identity allowlist — so it is the sink for both operational and the new degraded. But that step's body is unconditional and written for the operational case only:
:376— "This is an operational problem (Sigstore/Rekor/TUF reachability or the GitHub API)":377— "No maintainer action is required unless it persists; the check will resume automatically when upstream recovers":379/:111— "has been unable to complete for 3+ consecutive hourly runs"
For a catch-up stall all three are false: the monitor completed the pass and persisted progress, upstream is healthy, and recovery is gated on remaining decreasing (main.go:326-336), not on an upstream fix.
This defeats the purpose of the new classification for its core case. The whole point of classDegraded was to stop a permanently-behind catch-up reporting green — it now reports as a self-healing infra blip that maintainers are explicitly told to ignore.
Fix: branch the body on steps.monitor.outputs.classification (the security step at :272 already uses this pattern). For degraded, say the identity catch-up is not converging, name remaining, and give the actual remediations (raise the pass timeout or scan budget, or triage a held finding blocking the cursor). Make DEGRADED_TITLE neutral so the two states don't share an "unable to complete" title.
Same defect, documentation echo — the taxonomy is now inconsistent in four places:
.github/workflows/rekor-monitor.yaml:87-90— the triage block still reads "Operational / degraded (no page): a red hourly job with no issue is a transient infra blip … that self-heals … needs no action unless it persists." The alerting summary 50 lines above (:36-40) was updated and is correct, so the file contradicts itself. This is the block the degraded issue body itself points at (:298).tools/rekor-monitor/README.md:92-99,doc.go:56-60,main.go:428— all three enumerate exit codes withoutdegraded, and each closes with a sentence that partitions the space, so the omission reads as a claim rather than a gap. README's clean row ("the cursor advanced") is also now wrong for a partial catch-up pass, which is exit 0 with the cursor deliberately held atprev.docs/contributor/maintaining.md:64has the same stale framing as the triage block, while its new section at:203-212is correct.
✅ Confirmed non-issues (examined and cleared)
Several things a reviewer flagged that did not survive re-derivation, so you don't need to spend time on them:
- A malformed
.stallis not a wedge.readScanTrendis reached only fromrecordCatchUpProgresson a partial pass, which runs after the chunk loop has already persisted progress — so progress keeps advancing every run until the scan reaches head andadvanceCheckpointdeletes the bad file. Self-heals. (Two independent derivations agreed.) progress > endis not permanent either —end = cur.Size-1grows with the log, so the condition clears on its own once head passes the stale value, typically within an hour. Relatedly, the reset-and-rescan alternative to the hard refusal would not weaken the fail-closed property, becauseendis head: a full-window re-scan leaves nothing beyond it to miss, andscanIdentityis a pure read, so re-covering entries can only re-surface a finding, never suppress one.degradedtraces correctly end to end — the security step excludes it, the degraded step catches it,Close alert issue on successdoes not fire, and the streak jq (index("success") // length) is still correct.- First ~8 runs post-merge look right — run 1 scans ~950k (budget-bound, clean), run 2 finishes and advances. Degraded needs ~6-7 consecutive non-decreasing passes plus the workflow's 3-run streak, so there is no window where a working catch-up goes red.
- The soft-budget test is not wall-clock-flaky — it sets the headroom to 24h against a 1h context deadline, so the break fires deterministically with no sleeps.
maxScanEntriesPerRunat 2M is rarely binding but not dead; when it binds, the loop exits on the ordinary partial-pass path.actionlint/yamllintclean; no${{ }}inside anyrun:block;golangci-lint0 issues;go test -racepasses.
🔵 Nitpicks (fold into whatever commit touches these)
catchUpStalledError.Error()is the only new function at 0% coverage — it is the entire operator-facing payload of the degraded signal, so a swapped field would ship unnoticed.- The upload-step comment (
:253-257) says every advance "clears.stall, so the files are absent only when the run fails" — butresetScanTrendisos.Remove, so.stallis absent after every normal advancing run, i.e. the steady state. The round-trip itself is correct; only the comment is wrong. MakingadvanceCheckpointcallwriteScanTrend(0, 0)instead would make both companions symmetric and make that comment true. maintaining.md:209names only the advance-time reset; the primary one is inrecordCatchUpProgress— any pass whereremainingdecreases clears the counter and returns clean mid-catch-up, no advance needed.withScanBoundscoversscanChunkSizeandmaxScanEntriesPerRunbut notscanBudgetHeadroom, which the soft-budget test saves/restores by hand — a third process-global mutated outside the helper that exists to manage exactly that.parseFlagsvalidates onlytimeout > 0; any--timeout <= scanBudgetHeadroomputsscanDeadlinein the past at startup and silently degenerates to one chunk per run. Manual runs only — the scheduled workflow doesn't pass--timeout.
Summary
| Tier | Count |
|---|---|
| 🔴 Blocker | 0 |
| 🟠 Major | 2 |
| 🟡 Minor | 4 |
| 🔵 Nitpick | 5 |
Approving with comments. Worth doing before merge: the degraded issue body (Major 1, ~20 lines of text) and the three classDegraded tests (Major 2), since the exit-1 regression would fire a false security page. The stall-detector gap is the one worth a design conversation — it's the safety net for the exact problem this PR exists to solve. The untested-fixes cluster is one follow-up commit; the mutations that prove them convert directly into tests.
Nice work on this rework — the soft time budget in particular is a cleaner answer than the entry cap it replaced, and the reasoning left in the comments made the second pass much faster.
| // conclusion -- but it is still a non-security failure, so the workflow's | ||
| // degraded-issue path (which fires on any non-tamper/non-identity failure) | ||
| // opens a tracking issue after the usual consecutive-failure streak. | ||
| classDegraded classification = "degraded" |
There was a problem hiding this comment.
🟠 Major — Nothing pins classDegraded's two defining behaviours — proven by mutation
classDegraded exists precisely because it is (a) deterministic, so it must NOT be retried, and (b) a non-security failure, so it must exit 3 and never page. Neither is asserted anywhere.
Verified by running the suite under go test -overlay (no tracked file touched) with BOTH regressions applied at once: runWithRetry's gate changed so degraded is retried, and exitFor's class == classOperational || class == classDegraded narrowed so degraded exits 1. Result: ok github.com/NVIDIA/aicr/tools/rekor-monitor 1.687s — zero failures.
classDegraded / catchUpStalledError appear in tests only at observe_test.go:441-446 (one stderrors.As plus one classify() assertion). No TestRunWithRetry subtest, no TestClassify row, no TestRealMain row.
Blast radius: The exit-1 half is the sharp edge: a stalled catch-up would open the security alert issue and fire the Slack page — exactly the false page this classification scheme exists to prevent. The retry half silently triples the load a wedged monitor puts on Rekor.
Fix: Three small additions: a TestClassify row for &catchUpStalledError{} -> classDegraded; a TestRunWithRetry subtest whose runFn returns a *catchUpStalledError, asserting exactly one invocation; and a TestRealMain row asserting exit 3 plus a CLASSIFICATION=degraded line.
There was a problem hiding this comment.
Fixed in 2784b0d3. Added a TestClassify row (*catchUpStalledError -> classDegraded), a TestRunWithRetry subtest asserting exactly one invocation (degraded is deterministic, so it must not be retried), and a TestRealMain row asserting exit 3 + CLASSIFICATION=degraded. Mutating either defining behavior now fails the suite.
| if err != nil { | ||
| return err | ||
| } | ||
| if prevRemaining > 0 && remaining >= prevRemaining { |
There was a problem hiding this comment.
🟡 Minor — The stall detector is both defeatable by jitter and unbounded in absolute terms
Two halves of one defect.
Defeatable: the comparison is against prevRemaining — the immediately-prior pass — not a watermark. stall resets to 0 on any single decrease, and that same pass returns nil, so it also reports clean, which closes the degraded issue and resets the workflow's consecutive-failure streak. Worked example: remaining = 1000, 1100, 1200, 1150, 1300, 1400, 1350 — the monitor loses 350 entries of ground over six hours and reports green every run.
Unbounded: there is no absolute guard anywhere. remaining is computed, printed, handed to this function, and otherwise unused; nothing records how long the cursor has been held at prev. A perfectly converging catch-up can sit 5M entries and three weeks behind head with every run reporting CLASSIFICATION=clean, exit 0.
For the record on tiering: I do not think the oscillation regime is the expected shape — scanChunkSize (50k) is one chunk, not one run's throughput, and the code's own measurement is ~950k entries per ~40min budget against ~70k/hr growth, a ~13x margin. So this is a hole in the braces, not the belt.
Blast radius: This is the safety net for the silent-under-coverage gap the .stall companion was added to close. It works correctly for monotone divergence, but a jittery divergence never trips it, and no amount of convergence-checking bounds how far behind the identity scan may fall.
Fix: One change fixes the defeatable half: persist a best-ever (minimum) remaining watermark in field 1 instead of the last value, and increment stall whenever remaining >= bestRemaining. No file-format change — .stall already carries two integers. Separately, add an absolute trigger (degrade when remaining exceeds, say, 2x maxScanEntriesPerRun, or when a consecutive-partial-pass counter exceeds N — a third integer, no clock needed). The absolute bound is the more robust of the two, since jitter cannot defeat it.
There was a problem hiding this comment.
Fixed in 2784b0d3. The stall count now compares against a best-remaining low-water mark (field 1 of .stall), so an oscillating divergence cannot reset it. Added the absolute bound too: a third .stall field counts partial passes and degrades after maxCatchUpTotalRuns (72, ~3 days) to catch a glacial-but-monotone catch-up the watermark misses. It is set well above the passes a legitimate large backlog needs (<10 at ~1M/pass). I did NOT use the raw remaining > 2x maxScanEntriesPerRun variant because the current ~6.5M backlog legitimately exceeds that during normal catch-up and would false-page.
| // Read persisted scan progress up front: the rotation branch reports how far | ||
| // the abandoned prior-shard scan reached, and the same-shard path resumes from | ||
| // it. A present-but-unparseable companion is an error (never a silent reset). | ||
| progress, err := store.readProgress() |
There was a problem hiding this comment.
🟡 Minor — Hoisting readProgress above the early-return branches removed the paths that self-healed a corrupt .scan
At e7403a7b, store.readProgress() sat below the rotation branch and below !watchesIdentity() || !ok, immediately above if progress >= end. So on all three of those exit paths a corrupt .scan was never read, and each called advanceCheckpoint, whose writeProgress(0) overwrote the bad file. Self-healing.
At HEAD the read is hoisted here, above every branch, to populate out.abandonedProgress for the rotation report — which was the round-1 rotation fix. A malformed .scan now errors before rotation, before consistency-only, and before the first-run baseline.
Combined with readProgress erroring before any write, the if: !cancelled() upload re-publishing the poison, and the fetch step's sort_by(.created_at) | last re-selecting the artifact the failed run just wrote, the loop is fully closed and human-only.
Blast radius: Narrow reach — rotation is yearly, !watchesIdentity() never occurs in production, and an empty window is near-impossible at ~70k/hr — so the self-healing was already weak. But it existed and now does not, and there is no documented recovery. Note the round-1 fix for rotation reporting is what consumed the escape hatch.
Fix: Move readProgress back below the two early-return branches, and inside the rotation branch read it separately with a tolerant fallback — on error, log a warning and report the abandoned count as unknown rather than returning. out.abandonedProgress is purely a human-readable number in the rotation report; it should never be able to fail a pass, let alone permanently. Also worth adding a short "Recovering a wedged checkpoint artifact" runbook to maintaining.md (symptom, gh api -X DELETE the artifact, the coverage cost of re-baselining).
There was a problem hiding this comment.
Fixed in 2784b0d3. readProgress moved back below the rotation / consistency-only / baseline early returns, so those advance-and-overwrite paths never error on a corrupt .scan. The rotation report reads it separately via abandonedForReport, which logs a warning and reports the count as unknown (-1) on error rather than failing the pass. Also added the "Recovering a wedged checkpoint artifact" runbook to maintaining.md (symptom, gh api -X DELETE the artifact, the re-baseline coverage cost).
| // persisting the chunk so a run always makes at least one chunk of forward | ||
| // progress, and the partial-pass branch below leaves the checkpoint at prev | ||
| // to resume here next run. | ||
| if hasDeadline && time.Now().After(scanDeadline) { |
There was a problem hiding this comment.
🟡 Minor — A deadline shared across retry attempts misreports run outcomes in both directions
ctx is created once in realMain and all three runWithRetry attempts share it, while scanDeadline is derived from it here. One root cause, two opposite symptoms:
Productive run reported red. The budget is checked only after a chunk completes. If the in-flight chunk outlives the remaining headroom, ctx cancels inside scanIdentity, the scan error propagates, and attempts 2-3 die instantly in newMonitor on the same expired context. Exit 3, red — despite every completed chunk's progress being persisted. At ~24k/min a 50k chunk is ~2min against 5m headroom, so a <2x slowdown of the last chunk is enough.
Failed run reported green. Attempt 1 fails operationally at minute 41. Attempt 2 enters with the soft budget already blown but ctx not expired, so the top-of-loop ctx.Err() guard passes; one chunk runs, persists, and this check immediately breaks -> partial pass -> nil -> CLASSIFICATION=clean, exit 0.
Blast radius: No coverage loss in either direction — progress is monotone and no entries are skipped. The damage is to the operational signal: the first case opens a degraded issue for a healthy monitor, and the second erases a 41-minute genuine failure and supplies exactly the kind of single-pass reset that re-arms the stall counter and clears the workflow streak.
Fix: For the red case: at the scan-error site, if stderrors.Is(scanErr, context.DeadlineExceeded) AND at least one chunk landed (reached > scanFrom), break and fall through to partial-pass finalization instead of returning — everything after the break is local file IO that does not need a live context, and requiring one landed chunk means a genuinely hung first chunk still errors. For the green case: give each attempt its own sub-budget, or have realMain report the worst attempt's classification rather than the last.
There was a problem hiding this comment.
Fixed in 2784b0d3. Red case: at the scan-error site, a context.DeadlineExceeded with at least one landed chunk (reached > scanFrom) now breaks to the partial-pass finalization (local file IO, no live ctx needed) instead of returning operational. Green case: runWithRetry no longer retries an operational failure when time.Until(deadline) < scanBudgetHeadroom, so a near-expired retry cannot mask it with a trivial one-chunk partial. Added a TestRunWithRetry budget-guard subtest.
| // a shard rotation whose small new Size sits below a six-figure old-shard index). | ||
| // Safe because advanceCheckpoint is only reached once the window is fully scanned | ||
| // (or has nothing to scan), so resetting progress loses no unscanned coverage. | ||
| func advanceCheckpoint(store checkpointStore, prev, cur *tlog.Checkpoint) error { |
There was a problem hiding this comment.
🟡 Minor — Three more round-1 fixes are unpinned by tests — each reverts green
All three verified by go test -overlay against /tmp copies; no tracked file modified.
advanceCheckpointordering (this function, 60.0% coverage — the two error legs are the uncovered 40%). Reverting to the pre-rework order (store.writefirst,writeProgresslast) passes the suite. The 9-line rationale above it — that a mid-sequence failure must never pair a NEW checkpoint with stale old-window progress — has no test.- The restore hoist (
main.go:452). My first attempt at this mutation was misleading: movingrestoreback intorun()does fail a test, but only incidentally, because that test injects a fakerunFnso a restore inside the realrun()never executes. The semantically precise mutation — keep the hoist but re-restore before every attempt, i.e. the exact round-1 clobber bug — passes the whole suite. readScanTrend's numeric guards (checkpoint.go:184). Mutating the two numeric rejection legs toreturn 0, 0, nilpasses. This is the fail-closed guard for the stall detector itself: a leg returning(0,0)makesprevRemaining == 0every run, sostallresets forever and divergence never fires.
Blast radius: The rework is sound; its regression barrier is not yet built. Each of these can be silently undone by a future refactor. Impact is bounded (re-scanning, not skipping) except for #3, which silently restores the exact gap the stall detector was added to close.
Fix: One follow-up commit — the mutations above convert directly into tests. For #1, a table over a store whose path is made un-writable at each step, asserting the on-disk pair is never (new checkpoint, stale progress); both error legs get covered as a by-product. For #2, a realMain test with a valid zip containing .scan=N, a runFn that writes N+chunk and fails operationally on attempt 1, asserting attempt 2 reads N+chunk. For #3, a table over readScanTrend: absent, empty, one field, three fields, non-numeric/negative remaining, non-numeric/negative stall.
There was a problem hiding this comment.
Fixed in 2784b0d3. (1) TestAdvanceCheckpointResetsProgressBeforeWrite forces the checkpoint write to fail (path is a directory) and asserts the on-disk pair is never (new checkpoint, stale progress); both error legs get covered. (2) TestRealMainRestoresOnceAcrossRetries: valid zip with .scan=100, runFn writes 150 and fails operationally on attempt 1, asserts attempt 2 reads 150 (re-restoring would read 100). (3) The readScanTrend numeric guards are now covered by the expanded TestScanTrendRoundTrip table (absent, empty, legacy 2-field, 4-field, negative, non-numeric).
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
A few issues from a review of this change, concentrated on whether a run that cannot prove it scanned a range can still report green. Inline comments below; three items could not be anchored inline.
The degraded issue tells maintainers the wrong thing. main.go:377 adds degraded (exit 3) for a non-converging catch-up, and the degraded-issue step at .github/workflows/rekor-monitor.yaml:345 fires on any non-tamper, non-identity failure, so it catches degraded too. But the body at line 376 is hard-coded: "This is an operational problem (Sigstore/Rekor/TUF reachability or the GitHub API)... the check will resume automatically when upstream recovers." For a persistent throughput deficit nothing upstream will recover, and the issue explicitly tells maintainers no action is required while the unmonitored tail grows. Branching the body on steps.monitor.outputs.classification would fix it.
tools/rekor-monitor/README.md is stale after this change: the persistence contract does not mention the two companion files, and the exit-status table omits exit 3. That file is not modified here.
tools/rekor-monitor/monitor_test.go has a table case named "clean identity scan" that actually exercises the new partial-catch-up branch, and its substring assertion cannot distinguish the two. That file is also not modified here, so it is a pre-existing case whose meaning the new branch has changed.
| @@ -165,3 +297,69 @@ func selectCheckpointEntry(ctx context.Context, files []*zip.File, want string) | |||
| } | |||
There was a problem hiding this comment.
The sole-entry fallback can restore the .scan companion as the signed checkpoint and wedge the monitor.
advanceCheckpoint (main.go:349) calls store.writeProgress(0) and resetScanTrend() before store.write(). store.write delegates to the vendored rmfile.WriteCheckpointRekorV2, which returns early without writing when checkpoint.Size == 0. A first run against a size-0 shard therefore leaves only <CHECKPOINT_FILE>.scan on disk, and the persist step runs if: ${{ !cancelled() }} with if-no-files-found: ignore, so the artifact contains that single companion.
On the next run selectCheckpointEntry finds no entry whose base name is checkpoint_v2.txt, falls through to this len(files) == 1 branch, and returns the .scan entry — which restoreFromZip writes to the checkpoint path as a bare 0, and the checkpoint parser rejects.
The failing run then re-uploads that malformed checkpoint, so every subsequent scheduled run restores the same bad cursor: the hourly monitor stays red and provides no tamper or identity coverage until someone deletes the artifact by hand. Excluding known companion suffixes from this fallback, or never publishing a companion without a valid checkpoint, would close it.
| if err != nil { | ||
| return err | ||
| } | ||
| if prevRemaining > 0 && remaining >= prevRemaining { |
There was a problem hiding this comment.
The divergence detector counts only consecutive non-decreasing passes, so a catch-up that plateaus never trips it.
if prevRemaining > 0 && remaining >= prevRemaining { stall++ } else { stall = 0 } — one strictly-decreasing pass, even by a single entry, resets stall, and the degraded error only fires at stall >= maxCatchUpStallRuns (3). There is no absolute floor, no cumulative trend, and no bound on how long the signed checkpoint has been held at prev. A partial pass otherwise returns nil, which realMain turns into CLASSIFICATION=clean and exit 0.
If per-run throughput falls to roughly the log's growth rate, remaining oscillates — 500000, 500050, 500100, 499900, 500000 — and the single decreasing pass resets the counter each time. The monitor exits 0 hourly, the workflow's success step closes any open degraded issue, and several hundred thousand entries stay unscanned indefinitely while the checkpoint never advances.
docs/contributor/maintaining.md:211 says this closes the gap where a permanently-behind catch-up would otherwise report green indefinitely. As implemented it is a per-run comparison rather than a trend, so that gap is still open. The same mechanism also slows re-detection after a genuine divergence.
| # before it can advance the cursor or save a chunk (e.g. a first run whose | ||
| # consistency check errors); if-no-files-found: ignore covers that so a | ||
| # pre-advance failure does not fail the upload. | ||
| path: | |
There was a problem hiding this comment.
With the companion files, the artifact can now legitimately contain a companion without the checkpoint — the input that makes the sole-entry restore fallback in checkpoint.go misread a companion as the checkpoint. Worth pairing this upload with a guard that a companion is never published unless a valid checkpoint accompanies it.
| // to a full-window scan). A present-but-unparseable file is an error rather than | ||
| // a silent reset, which would re-scan (and possibly re-time-out) the backlog. | ||
| func (s checkpointStore) readProgress() (int64, error) { | ||
| data, err := os.ReadFile(s.progressPath()) //nolint:gosec // path is a workflow-controlled constant |
There was a problem hiding this comment.
Companion files are read with unbounded os.ReadFile, while the equivalent zip-extraction path caps the read at 4 KiB. These files hold a single integer, so the asymmetry means a corrupted or hostile on-disk companion is read in full into memory. os.Open plus io.LimitReader would match the zip path.
| return 0, errors.Wrap(errors.ErrCodeInternal, "failed to read scan-progress file", err) | ||
| } | ||
| trimmed := strings.TrimSpace(string(data)) | ||
| if trimmed == "" { |
There was a problem hiding this comment.
A blank or zero-byte .scan (or .stall) companion silently resets progress to 0 rather than erroring, which contradicts the invariant documented a few lines above. Progress resetting to zero is indistinguishable from a genuine fresh start, so a truncated companion quietly discards a scan position instead of failing loudly.
| // passes, so the log is outpacing the scan. It is a distinct type so classify can | ||
| // map it to classDegraded (deterministic, not retried) rather than a retryable | ||
| // operational failure. | ||
| type catchUpStalledError struct { |
There was a problem hiding this comment.
catchUpStalledError is a bare Go error type built with fmt.Sprintf, returned unwrapped at line 335. Every other terminal error in this tool comes from pkg/errors with a code.
The distinct-type requirement is preserved if it is wrapped — errors.Wrap(errors.ErrCodeUnavailable, ..., &catchUpStalledError{...}) — since stderrors.As traverses the pkg/errors chain, which classify already relies on for proof.RootMismatchError. As written the degraded path is the only failure producing an uncoded error, so it is invisible to code-based handling. No runtime misbehaviour: classification and exit 3 are still correct.
| // conclusion -- but it is still a non-security failure, so the workflow's | ||
| // degraded-issue path (which fires on any non-tamper/non-identity failure) | ||
| // opens a tracking issue after the usual consecutive-failure streak. | ||
| classDegraded classification = "degraded" |
There was a problem hiding this comment.
The new degraded classification (exit 3) is not reflected in the tool's exit-status contract table, so the documented contract no longer matches the implemented one.
| // TestCheckpointStoreRestoreProgress verifies the scan-progress companion round | ||
| // trips through the artifact zip: when present it is restored (so the next run | ||
| // resumes), and when absent restore is a no-op (progress reads as 0). | ||
| func TestCheckpointStoreRestoreProgress(t *testing.T) { |
There was a problem hiding this comment.
TestCheckpointStoreRestoreProgress covers only present and absent companions. Nothing exercises a malformed (non-integer) .scan, which is the input that drives the silent reset-to-zero in checkpoint.go.
| // loudly rather than reporting clean forever. | ||
| // - monitor (monitor.go) owns the resolved v2 shards and the watched identity, | ||
| // and exposes the two checks: checkConsistency and scanIdentity. | ||
| // - outcome (monitor.go) carries the result of one pass so run() can report it |
There was a problem hiding this comment.
The package doc still describes run() as performing the restore, and still documents exit 3 as operational-only. Both changed in this PR.
| return | ||
| } | ||
| if !o.hasFindings() { | ||
| if o.scanned && !o.caughtUp { |
There was a problem hiding this comment.
The o.scanned operand in this catch-up status branch is redundant — the enclosing early return already guarantees it is true.
…ark, self-heal, tests) - degraded issue body + docs branch on classification instead of the operational 'transient, ignore unless it persists' text; neutral title - stall detector: compare against a best-remaining watermark (defeats jitter) plus an absolute partial-pass bound for a glacial catch-up - restore the corrupt-.scan self-heal on rotation/baseline/consistency-only paths (read progress only on the scan path); add a recovery runbook - deadline: a productive run whose last chunk overruns headroom finishes as a clean partial; do not retry an operational failure with no budget left - pin classDegraded (not retried, exit 3), advanceCheckpoint ordering, restore-once-no-clobber, and readScanTrend numeric guards with tests Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
|
@njhensley thanks for the round-2 approve and the mutation testing — all of it is addressed in Major 1 (degraded issue body/docs, no inline anchor): the degraded-issue step now branches its body on Full suite + golangci-lint + actionlint + yamllint green; coverage 82.7%. |
3 participants
Summary
Make the Rekor monitor's identity scan resumable so a large backlog window (from a multi-hour outage or a held cursor) is caught up across several hourly runs instead of re-scanning — and timing out on — the whole window every pass. Also re-enable the scheduled cron.
Motivation / Context
The identity scan is linear in the window size. On 2026-07-24 the window reached ~1.17M entries and exceeded even the 45-min pass deadline, so the monitor timed out (
operational) every run and could not self-heal without a re-baseline (which skips the backlog — a coverage gap). This makes it catch up instead, keeping full coverage.Follows #1897 / #1900 / #1903.
Type of Change
Component(s) Affected
docs/,examples/)tools/rekor-monitor+.github/workflows/rekor-monitor.yamlImplementation Notes
checkConsistency -> headproof is O(log n), never the bottleneck) and advances only once the scan reaches head. A newscannedToindex is persisted in a<checkpoint>.scancompanion carried in the same artifact.(scannedTo, head]inscanChunkSize(50k) chunks up tomaxScanEntriesPerRun(300k), persisting progress after each clean chunk, then resumes there next run. A ~1.2M backlog clears in a handful of runs, each within budget; the log grows ~70k/hr, far below the cap, so it converges.catching up, N remaining). A finding still halts at its chunk and its progress is not persisted, so it re-detects until triaged — and because a finding halts before later chunks, a partial-clean pass never coexists with an open finding alert (socleanclosing stale issues stays correct; no new classification needed).schedule/cronblock (commented out on 2026-07-24 to pause the wedged monitor).Testing
New tests: multi-run catch-up (progress persists, checkpoint advances only when caught up, chunks contiguously cover the window), finding-halts-catch-up (no advance, progress held at the last clean chunk), progress round-trip (write/read + absent/blank = 0), and restore-companion round-trip through the artifact zip. Existing
TestObservecases still pass (small windows scan in one pass). Coveragetools/rekor-monitor: prior ~clean → 82.1%.Risk Assessment
Rollout notes: On merge, the resumed scheduled runs catch up the current wedged backlog over several passes (each
clean, exit 0, no page) until the checkpoint advances to head, then return to steady-state hourly windows.Checklist
make testwith-race)make lint) — golangci + yamllint + actionlint cleangit commit -S)