fix(synccompactor): preserve budget for full expansion fallback - #1111
fix(synccompactor): preserve budget for full expansion fallback#1111manojacs wants to merge 3 commits into
Conversation
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if err != nil { | ||
| return nil, fmt.Errorf("incremental expansion: open base graph store: %w", err) | ||
| } | ||
| closeCtx, cancelClose := context.WithTimeout(context.WithoutCancel(ctx), dotc1z.FinalizeTimeout()) |
There was a problem hiding this comment.
🟡 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.
| return false, err | ||
| } | ||
| return c.finishIncrementalExpansion(ctx, newSyncId, base, verification) | ||
| return c.finishIncrementalExpansion(classificationCtx, newSyncId, base, verification) |
There was a problem hiding this comment.
🟡 Suggestion: finishIncrementalExpansion (and the restoreEndedSync calls above) now receive classificationCtx — the tightest-deadline context in this function, and one that is normally already expired here because the walk ran on walkCtx with up to 4× the classification budget. This is correct today only because both helpers immediately do context.WithTimeout(context.WithoutCancel(ctx), dotc1z.FinalizeTimeout()), so the deadline is stripped and only the zap logger survives. It contradicts the PR's own "keep restore and finalization detached from the expired classification context" invariant at the call site, and any future non-detached ctx use inside those helpers fails silently on the commit path. Passing walkCtx would make the intent match the code without changing behavior.
| // incrementalClassificationBudgetPercent caps the cheaper, speculative | ||
| // pre-write phase at one quarter of the remaining run duration. If it declines, | ||
| // the more expensive full expansion retains most of the budget; if it succeeds, | ||
| // the grant walk may still use the full run deadline. Mandatory detached | ||
| // restore/finalization runs outside this cap on contexts bounded separately by | ||
| // dotc1z.FinalizeTimeout(). | ||
| const incrementalClassificationBudgetPercent = 25 |
There was a problem hiding this comment.
🟡 Suggestion: this introduces a new default-path decline trigger for every Pebble incremental compaction, but 25% is a hardcoded constant with no env/option override and no dedicated outcome reason. When the cap fires, the run logs fell_back/base_graph_error or fell_back/incremental_error with a context.DeadlineExceeded buried in zap.Error, so you cannot measure how often the cap — rather than a real decline — is the cause, nor tune it in the field. Consider a distinct reason (e.g. classification_budget_exceeded, set when errors.Is(err, context.DeadlineExceeded) && classificationCtx.Err() != nil && walkCtx.Err() == nil) plus an env knob.
Separately, per docs/BUG_CATCHING.md cost contracts: classification is O(grants) on the merged artifact (base graph load + full PendingExpansion enumeration + changedEntitlementIDs), and the tests here use 10s run durations over tiny fixtures — TestIncrementalClassificationTimeoutFallsBackWithRunDuration injects an already-expired deadline rather than measuring real classification cost. So nothing in the PR establishes that classification fits in 25% at whale scale. If it does not, a compaction that previously succeeded incrementally now declines and runs the strictly more expensive full expansion with only 75% of the budget — the opposite of the change's goal. A benchmark or measured classification-time/run-duration ratio would pin the constant.
General PR Review: fix(synccompactor): preserve budget for full expansion fallbackBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff ( Risk triage (per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| // Base extraction is part of speculative classification. It used to run on | ||
| // the unbounded parent context and could consume the entire fallback | ||
| // budget before discovering that the base had no reusable graph. | ||
| baseGraph, loadErr := c.loadIncrementalBaseGraph(classificationCtx) |
There was a problem hiding this comment.
🟡 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.
| case errors.Is(err, context.DeadlineExceeded): | ||
| logIncrementalOutcome(ctx, "fell_back", "classification_timeout", append(fields, zap.Error(err))...) |
There was a problem hiding this comment.
🟡 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.
| if percent := c.incrementalClassificationBudgetPercent; percent < 0 || percent >= 100 { | ||
| return nil, nil, fmt.Errorf("incremental classification budget percent must be between 1 and 99, got %d", percent) | ||
| } |
There was a problem hiding this comment.
🟡 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.
1 participant
Problem
Incremental grant expansion is speculative until the compactor has loaded a reusable base graph and classified the merged expandable rules. Previously, base loading,
PendingExpansionscanning, and restore work could consume the run budget before the remaining-duration check.A decline could therefore follow this sequence:
This addresses item 2 from the review of #1013.
Changes
WithIncrementalClassificationBudgetPercent.PendingExpansionenumeration, source-entitlement reads, and changed-entitlement classification to that bounded context.dotc1z.FinalizeTimeout().classification_timeoutseparately from real base-graph or incremental errors.not_attempted/run_duration_exhaustedwhen no budget remains before base I/O.The resulting flow is:
Tests
Added coverage for:
ResumeSync, emitting the dedicated reason, followed by detached restore and successful full expansion;Validation:
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.