fix(synccompactor): preserve budget for full expansion fallback

[manojacs]

Problem

Incremental grant expansion is speculative until the compactor has loaded a reusable base graph and classified the merged expandable rules. Previously, base loading, PendingExpansion scanning, and restore work could consume the run budget before the remaining-duration check.

A decline could therefore follow this sequence:

incremental setup consumes the run budget
  -> incremental expansion declines
  -> full expansion has no time remaining
  -> compaction fails instead of producing an artifact

This addresses item 2 from the review of #1013.

Changes

• Run cheap eligibility checks before opening the base artifact.
• Limit speculative, pre-write classification to 25% of the remaining run duration by default.
• Allow callers to tune that share with WithIncrementalClassificationBudgetPercent.
• Charge base loading, PendingExpansion enumeration, source-entitlement reads, and changed-entitlement classification to that bounded context.
• Keep the committed incremental walk and invariant checks on the full run context, so a valid fast path is not stopped merely to reserve time for a more expensive fallback.
• Cancel the classification timer before starting full expansion.
• Compute the full-expansion duration once, validate it before fallback setup, and pass the same value to the syncer.
• Pass the stable run context to restore/finalization; those helpers explicitly detach and apply dotc1z.FinalizeTimeout().
• Start a fresh detached timeout immediately when closing a reopened base store, so artifact reads cannot consume its cleanup budget.
• Emit classification_timeout separately from real base-graph or incremental errors.
• Emit classification duration and the configured budget percentage so the 25% default can be validated against production workloads.
• Emit not_attempted/run_duration_exhausted when no budget remains before base I/O.

The resulting flow is:

cheap checks
  -> classification, capped at 25% of remaining time by default
       -> decline or timeout: restore and run full expansion
       -> safe: run incremental expansion with the full run deadline

Tests

Added coverage for:

• default and configured classification-budget arithmetic;
• rejecting invalid configured percentages;
• skipping base I/O when the run budget is already exhausted;
• a normal incremental decline completing full expansion and producing a valid artifact;
• an actual classification deadline expiring after ResumeSync, emitting the dedicated reason, followed by detached restore and successful full expansion;
• restore succeeding with an already-expired classification context;
• base-store close receiving a fresh detached finalization deadline even when its parent has already expired.

Validation:

go test ./pkg/synccompactor ./pkg/sync/expand -count=1
go test -race ./pkg/synccompactor -run 'Test(IncrementalClassification|CloseIncrementalBaseStore|RestoreEndedSync|ExpandGrantsSkips|IncrementalDecline)' -count=1
go vet ./pkg/synccompactor ./pkg/sync/expand
git diff --check

All pass locally.

Related work

#1105 removes the redundant base reopen on the normal Pebble fold path. This change remains responsible for budget ordering and for bounding classification on fallback/rebuild paths.

[linear-code]

DAT-857

[github-actions]

🟡 Suggestion: closeCtx is created here at open time but only used after LatestFinishedSyncOfAnyType and GraphFromStore (both O(artifact) reads), so its remaining budget at each store.Close(closeCtx) is FinalizeTimeout() - (time spent reading) and can already be expired. BATON_C1Z_FINALIZE_TIMEOUT is operator-tunable in whole seconds, so a low setting makes the close fail on an expired context — masking the real outcome behind "close base graph store" and leaking exactly the extracted temp data this change set out to protect. restoreEndedSync and finishIncrementalExpansion build their detached context immediately before use; doing the same here (a small closeBaseStore(ctx, store) helper, or one context.WithTimeout(context.WithoutCancel(ctx), ...) per call site) keeps the full finalize budget available at close.

[github-actions]

General PR Review: fix(synccompactor): preserve budget for full expansion fallback

Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 729390986af8.
Review mode: full
View review run

Review Summary

Scanned the full PR diff (pkg/synccompactor/compactor.go, new incremental_budget_test.go) for security and correctness; no dependency, proto, or exported-struct changes are present, and the one new exported symbol (WithIncrementalClassificationBudgetPercent) is purely additive. Three of the four prior findings look genuinely addressed: base-store close now runs on a fresh detached dotc1z.FinalizeTimeout() budget via closeIncrementalBaseStore, the restore/finalize helpers are now handed walkCtx rather than the classification context, and cap-induced declines now carry a dedicated classification_timeout reason plus duration/percent fields and a percent override. I verified the newly narrowed classification paths cannot silently truncate classification input — PaginateGrantsByNeedsExpansion checks ctx.Err() and surfaces it through the iterator, AdaptNotFound preserves wrapping, and every post-ResumeSync error path still calls restoreEndedSync — so no wrong-artifact risk was found. Remaining findings are observability and budget-coverage gaps, not regressions.

Risk triage (per docs/BUG_CATCHING.md §2): silence — partial yes (budget misallocation surfaces only as a run-duration failure and as outcome logs); durability — no (no change to c1z contents, wire types, or serialized state on the success path); uncontrolled dimensions — yes (correctness of the 25% split depends on artifact size and wall-clock, which fixtures undersample); consumer distance — low (same process). Verdict: MEDIUM. The PR does ship a permutation-ish table of budget cases (incremental_budget_test.go), but the instrument that would give real coverage — a whale-scale timing benchmark establishing that classification actually fits inside 25% — is still absent, which the prior review already recorded as a finding and is not re-raised here.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/synccompactor/compactor.go:1317 — on Pebble (the only engine reaching this path) classificationCtx does not bound base extraction: unpackExistingPebbleC1Z/ExtractEnvelopePayload take no context.Context, so the O(artifact) decompress the comment names as the motivating cost can still consume the whole run budget before fallback.
• pkg/synccompactor/compactor.go:1342 — the context.DeadlineExceeded arm also catches walkCtx/runCtx expiries from the committed walk and invariant run, so a run-duration timeout is reported as classification_timeout with the budget-percent field, misdirecting operators to the cap.
• pkg/synccompactor/compactor.go:297 — an explicit WithIncrementalClassificationBudgetPercent(0) passes validation and silently resolves to 25, contradicting both the error message and the option's documented 1-99 range.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/synccompactor/compactor.go`:
- Around line 1317: `loadIncrementalBaseGraph(classificationCtx)` is documented as bounding
  speculative base extraction, but on the Pebble engine the extraction is not cancellable.
  `dotc1z.NewStore` -> `pebbleDriver.OpenStore` (pkg/dotc1z/pebble_store.go:56) calls
  `unpackExistingPebbleC1Z(...)`, which takes no `context.Context` and calls
  `formatv3.ExtractEnvelopePayload(f, dbDir, opts...)` (pkg/dotc1z/format/v3/indexed.go:681),
  also context-free. The classification deadline only takes effect later, at
  `pebble.Open(ctx, dbDir)` and `InitCurrentSync(ctx)`. Result: decompressing a whale base
  artifact can still burn the entire run duration before the fallback check runs, which is
  the failure this PR set out to prevent. Fix by threading a `context.Context` through
  `unpackExistingPebbleC1Z` and `ExtractEnvelopePayload` (checking `ctx.Err()` between
  payload chunks), or by measuring extraction and declining before the remaining budget is
  gone. Add a test where the classification budget is exceeded during base extraction, not
  only after `ResumeSync` as `TestIncrementalClassificationTimeoutFallsBackWithRunDuration`
  does today.
- Around line 1342-1343: the `case errors.Is(err, context.DeadlineExceeded)` arm labels the
  outcome `classification_timeout` for any deadline error returned by
  `expandGrantsIncremental`, but that function runs `ExpandChanges(walkCtx, ...)` (line 888)
  and `runIncrementalInvariants(walkCtx, ...)` (lines 866 and 905) on `walkCtx`, which is
  `runCtx`. When the overall run duration expires during the committed walk, the log claims
  the 25% classification cap caused the fallback and attaches
  `incremental_classification_budget_percent`, which is misleading. Distinguish the sources:
  report `classification_timeout` only when `classificationCtx.Err() != nil &&
  runCtx.Err() == nil`, and add a distinct reason such as `run_duration_timeout` for the
  other case.
- Around line 297-299: validation is `percent < 0 || percent >= 100`, so an explicit
  `WithIncrementalClassificationBudgetPercent(0)` is accepted and
  `resolvedIncrementalClassificationBudgetPercent()` (line 1275) silently returns 25. Both
  the returned error text and the option's doc comment state the valid range is 1-99, so a
  caller that computes a percent which rounds down to 0 gets the default instead of the
  promised validation error. Either record whether the option was applied (store an
  `*int`, or add a `budgetPercentSet bool`) and reject an explicit 0, or change the option's
  doc comment and the error message to state that 0 selects the default.

[github-actions]

No blocking issues found.

[github-actions]

🟡 Suggestion: On the Pebble engine — the only engine that reaches this path — classificationCtx does not actually bound base extraction. dotc1z.NewStore → pebbleDriver.OpenStore → unpackExistingPebbleC1Z → formatv3.ExtractEnvelopePayload take no context.Context (pkg/dotc1z/pebble_store.go:69, pkg/dotc1z/format/v3/indexed.go:681), so the O(artifact) decompress named in the comment above runs to completion regardless of the deadline; the cap only starts biting at pebble.Open(ctx, …). For a whale base the run budget can still be fully consumed before fallback — the exact failure this change targets. Consider threading ctx into the unpack path, and adding a test where extraction (not post-ResumeSync work) blows the classification budget.

[github-actions]

🟡 Suggestion: This arm also catches deadline errors that came from walkCtx (== runCtx), not the classification cap — ExpandChanges(walkCtx, …) (line 888) and runIncrementalInvariants(walkCtx, …) (lines 866/905) both return their ctx error through here. A run-duration expiry during the committed walk is then logged as classification_timeout alongside incremental_classification_budget_percent, pointing operators at the 25% cap for a failure the cap did not cause. Distinguish the two, e.g. classificationCtx.Err() != nil && runCtx.Err() == nil for classification_timeout, and emit a separate run_duration_timeout reason otherwise.

[github-actions]

🟡 Suggestion: percent < 0 lets an explicit WithIncrementalClassificationBudgetPercent(0) through, and resolvedIncrementalClassificationBudgetPercent then silently returns 25 — contradicting both this error message and the option's doc comment ("Values must be in the range 1-99"). A caller computing the percent (e.g. budgetMs*100/totalMs rounding to 0) gets the default instead of the validation error the API promises. Either track whether the option was applied (pointer or set bool) and reject an explicit 0, or document that 0 selects the default in WithIncrementalClassificationBudgetPercent.

[github-actions]

No blocking issues found.