perf(codemap): avoid repeated full-store scans when publishing manifests#488
perf(codemap): avoid repeated full-store scans when publishing manifests#488morluto wants to merge 35 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
baron
left a comment
baron
left a comment
There was a problem hiding this comment.
Intent Analysis
Problem: Root-manifest persistence can monopolize a CPU core during indexing bursts: each file completion published separately and each publication triggered up to six store-wide manifest scans/decodes. Repeated semantic corruption also drove quarantine/regeneration loops, with decode failures collapsed into generic "corrupt" (issue #466).
Before → After: One work item per publication with array-based queue ops → bounded FIFO batches coalescing compatible revisions, ≤64 items plus a byte limit (WorkspaceCodemapBindingEngine.swift L6596–6631). Up to six store scans per publication → one quota scan + one post-commit authority scan on the normal path (CodeMapRootManifestStore.swift L1151–1184, L1271–1322); same-size access refreshes bypass store-wide reconciliation (L787–805). Post-commit maintenance failure becomes bounded retryable debt instead of misreporting a durable write as failed (L1325–1350, L1372–1387). Decoded snapshots cached behind file-identity + SHA-256 verification; decode failures gain typed attribution and regeneration backpressure — on the direct-load path only (see Findings).
Evidence: Issue #466 profiler stacks and acceptance criteria; ~17 new focused tests covering batching persistence/reload (CodemapBindingEngineManifestWriteTests.swift L200–249), scan bounds, eviction, post-rename mutation, cache invalidation, atomic publication, quota protection, bounded retry, waiters, and corruption attribution. Two independent code-first reviews converge on these facts.
Confidence: High on main-path behavior and state-machine correctness; Medium on end-to-end performance magnitude (tests count scans; no near-capacity profiling artifact); High on the maintenance-scan gap below (exact code trace, unrefuted, partially corroborated by both reviews).
Red Flags: Stacked on still-open #487 (diff includes its commits, ~2.2k lines combined). CI is red at head 3b94c09: Build and Test (app shard 2) FAILURE, three checks pending. PR auto-closes #466 while one of its acceptance criteria is unmet on the maintenance path. No maintainer review discussion yet.
Findings
High — Maintenance-discovered semantic corruption bypasses typed attribution and backpressure. loadCurrentManifest records typed decode failures and regeneration state (CodeMapRootManifestStore.swift:476–483), but the store-wide scan path catches CodeMapRootManifestDecodeFailure and reduces it to generic .corrupt without recording it (:1825–1828); a mutating maintain() scan then quarantines the entry (:1680–1692). No regenerationFailures update means maintenance-driven quarantine/regeneration loops are not backpressured — the loop #466 lists as an acceptance criterion. New tests cover direct-load corruption only, not scan-discovered corruption.
Low — Retry-exhausted maintenance debt and decode accounting are not production-observable. Debt status is #if DEBUG-only and decodeFailureAccounting() has no production consumer (:312–341; only tests call it). Not blocking alone, but it compounds the High finding: the path that loses attribution also has no production surface where the loss would show. Recommend a production diagnostic counter.
Process — Stacked boundary and failing CI. #487 must land first and #488's delta be reconciled; Build and Test (app shard 2) is FAILURE with three checks pending, so required CI is not green and the failure must be dispositioned (known concurrency flakes exist, but that is a rerun-and-confirm step, not an assumption).
Maintainer-guidance check
- User impact/invariant: Indexing/maintenance stays bounded without weakening atomic publication, quota enforcement, exact writer authority, or FIFO revision ordering — all preserved on the main path with strong focused coverage (commit boundaries, exact-target protection, unload races, absorbed waiters, bounded retry).
- Root-cause confidence: Confirmed for scan amplification and per-item publication; incomplete for the repeated-corruption loop (scan path loses typed context).
- Authority: Locked on-disk manifest snapshot under the cross-process anchor lock remains the single persistence authority; engine revision/proof state gates publication. Unchanged.
- State-safety: No concrete data-loss or stale-publication path found by either review; the #487 Sentry "dropped pending mutations" claim was traced and is not valid (
deferredWorkretains failed batches plus the remaining FIFO prefix in order). - Scale/observability: Scan/batch counters are bounded and surfaced; maintenance-debt state and typed decode accounting are not. No integrated near-capacity (~256-manifest) burst measurement.
- Recommended scope: Keep performance + state-machine changes together. Close the scan-path attribution/backpressure gap here, or explicitly split it into a still-open follow-up and de-link "closes #466". Locator-store hotspot correctly deferred to #480.
- Validation boundary: Add a test where
maintain()discovers repeated semantic corruption and verify typed attribution + cooldown; run focused manifest-store/writer suites; required CI green including the shard-2 failure. Near-capacity profile is valuable but advisory.
Verdict
REQUEST_CHANGES. Three defensible grounds: (1) scan-discovered semantic corruption escapes both attribution and backpressure while the PR auto-closes the issue whose acceptance criteria include that loop — a concrete gap in the mechanism this PR exists to fix; (2) CI is red at the current head (shard 2 FAILURE, three checks pending); (3) the stacked #487 boundary is unresolved, so the mergeable diff is not the intended delta. The core design is sound, evidence-led, and well tested; once the maintenance-scan gap is closed (or #466 explicitly split), #487 lands, and CI is green, this converts cleanly to approval.
…etries Previously, a `WorkspaceCodemapBindingEngine` manifest write failure would swallow the in-flight batch plus all queued work into `deferredWork`, fail all waiters, and leave the deferred batch unrecovered. This caused manifest records to be lost on root unload/restart and allowed unbounded growth if new mutations kept arriving while the writer repeatedly failed. Now the writer: - Keeps `deferredWork` and its waiters after a failure. - Schedules an autonomous retry with a bounded delay. - Tracks `deferredFailureCount` and drops the oldest deferred batch after `maximumManifestWriterDeferredAttempts` consecutive failures. - Enforces a hard `maximumManifestWriterDeferredItemCount` cap by evicting the oldest excess items on each failure. - Cancels any pending retry task when new work arrives (or on shutdown). - Resets `deferredFailureCount` only when the oldest deferred batch is explicitly dropped, so retries are counted across attempts. `discardManifestWorkItems` detaches affected waiters, removes the matching `pendingManifestChanges`, and clears the dirty state when a pipeline has no remaining pending changes. Update `testFailedManifestBatchResolvesEveryAbsorbedRevisionOnce` to expect the retry success counts, and add regression tests for one-shot recovery with reload, repeated transient recovery, and bounded abandonment after the max failure budget.
…ruption `inspectManifestDescriptor` now records typed `CodeMapRootManifestDecodeFailure` counts and regeneration failures for semantic corruption encountered during store-wide scans, instead of collapsing it to an untyped `.corrupt` result. `recordRegenerationFailure` now accepts an optional `authority` and a digest, so scan paths that do not know the current authority can still contribute to backpressure. `waitForRegenerationBackpressure` and `clearRegenerationFailure` now treat a `nil` recorded authority as matching the current authority, so the first write after a scan-discovered failure observes the backoff until the manifest is successfully repaired. Add `testMaintenanceRepeatedSemanticQuarantineDelaysAndBackpressuresRepair` which verifies `maintain()` discovering repeated `.contributionValidation` failures records the typed failure, applies the regeneration backoff, and allows the repair write after the delay.
…on' into perf/manifest-store-reconciliation
|
Addressed Baron findings and the exact hosted failures through pushed commits |
|
Fresh exact-head shard 2 found a compile error in the new maintenance regression: |
|
Fixed the current exact-head shard-3 failure in |
…thority
The early `guard let authority else { return }` prevented
`recordRegenerationFailure` from recording scan-discovered failures that
arrive without a resolved authority. Remove the guard and extend the
matching condition so a nil authority can either reuse an existing state
or create a new one, matching the behavior of authority-bearing failures.
…re assertion The codemap projection preload (a background task) can resolve eligibility for the non-git root before the getCodeStructure call, caching the result. When getCodeStructure runs, it finds the cached result and skips the eligibility check, so no git commands are recorded. The manifest writer batching changes in this PR affect timing, causing the preload to complete earlier and exposing this race. Add invalidateCodemapEligibilityForTesting hook to WorkspaceFileContextStore that waits for pending preload to settle and then detaches the codemap session. Call this hook before the diagnostics reset in testThreeRootSessionScopeReplacesCanonicalGitRootAndPreservesIndependentNonGitRoot to ensure getCodeStructure triggers a fresh eligibility check.
2 participants
Problem
Writer batching reduces how often manifests are published, but each remaining publication still performed excessive store-wide work.
One publication could invoke reconciliation twice, with three complete scans per reconciliation. Access refreshes also reused the scan-heavy publication path for every namespace. Near the store limit, repeated enumeration, decoding, quota accounting, and LRU reconciliation amplified indexing cost.
Together with #487, fixes #466.
Approach
Keep the authority and quota checks, but separate them by purpose: authoritative accounting before publication, exact target and terminal quota validation after publication, and full cleanup only for explicit maintenance or anomalies.
Decoded manifests are reused only after authoritative enumeration, owned-path validation, file-identity checks, and fresh checksum verification. A single maintenance-debt state records post-commit work that still needs attention without treating an already durable publication as unwritten.
Changes
Performance effect
A normal manifest publication now performs two authoritative store scans instead of six: one before publication and one after the rename.
Access refreshes no longer run store-wide quota accounting for every touched namespace. Validated cache entries avoid decoding unchanged manifests after path, file-identity, and checksum verification. This reduces full-store scan count by two-thirds on the normal publication path while preserving quota, ownership, and stale-writer checks.
Safety preserved
Testing
Coverage includes scan bounds, cache invalidation, coarse timestamps, external replacement, quarantine, eviction, quota enforcement, pending maintenance, public maintenance, and clean publication superseding obsolete debt.
CodeMapRootManifestStoreTests: passedRepoPromptproduct build: passed