fix(code-index): preserve and bound live migration replay - #882
Conversation
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a239f44418
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Architecture review of Filed #886 for a concrete simplification exposed by this patch: The existing inline review already covers the two more serious concerns, so I am not filing duplicate issues for them: the corpus-wide owned identity maps during legacy restore, and replacing the interactive deadline with a very long background budget without a cost/progress proof. Keep the memory acceptance attached to #811/#852 and the semantic lifecycle acceptance attached to #753/#837. Successful completion on the reported 3,863-file profile is useful evidence, but it does not by itself bound peak memory or prove prompt cancellation of a stalled restart. For the compatibility acceptance in #886, retain historical-writer fixture bytes with real marker-bearing evidence. The added test formed by removing Source/diff review only; I did not run the reported tests or reproduce the operator journey. |
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Two P1s remain unresolved and should block this PR from landing into #707:
-
Legacy restore must not allocate corpus-wide identity maps. The current compatibility decoder retains the decoded files + evidence DOM and then builds owned
BTreeMap<String, String>entries for every symbol/chunk. Replace that with marker parsing/direct indexed resolution (or another demonstrably bounded lookup) so migration cost is proportional to evidence actually encountered, not total corpus identities. This is especially important because #852 already establishes that simultaneous generation representations are the dominant memory risk. -
Do not replace a 30s timeout with the 168-hour background budget. Corpus-scale restart can have a different budget from an interactive request, but it still needs a measured finite bound plus progress/cancellation checkpoints. The acceptance should prove a stalled phase releases the semantic worker promptly; successful completion of the 3,863-file operator profile proves the happy path, not bounded failure behavior.
Also complete #886 while touching this codec: share the common paged/legacy descriptor validation between the full parser and streaming retention reader without merging their authentication responsibilities.
The missing changeset is not independently concerning if this PR is intentionally absorbed into unreleased #707 before release; do not add release metadata merely to silence the bot in that case.
After the fixes: rerun the focused codec/usecase tests already listed in the PR, add a peak-memory measurement for the legacy migration fixture, add a cancellation/stall test for restart, and get the forked-workflow runs approved/green. Current workflow results are action_required, so there is no CI evidence on this head yet.
…n-plan-reopened' into fix/pr707-live-validation-20260904
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Current-head deep review (316e8e73f). The earlier corpus-wide identity-map P1 and 168-hour authority P1 have been materially addressed: legacy markers now resolve directly from persisted indices, descriptor layout validation is shared, explicit null is rejected, and background graph authority is finite at 15 minutes. Those prior objections should not be carried forward unchanged.
One new merge blocker remains in the current branch-add wait implementation: BRANCH_GENERATION_SEATING_POLL_INTERVAL = 10ms drives the whole exact-generation wait loop, and the loop calls dashboard_freshness(...).await before every select. A long but healthy generation can therefore execute ~100 freshness probes/sec for up to 30 minutes (~180k probes per waiter), plus serving-scope reads, with unrelated publication wakeups causing more iterations. Multiple branch-add callers multiply that background demand. The comment says polling exists only to bridge the narrow publication-before-seating race; scope it to that race instead of polling the entire generation lifetime. Prefer a seating/serving-generation notification tied to the existing owner; otherwise only start a bounded, slower confirmation poll after a matching publication event. Keep the hard deadline and idle-progress semantics.
I would also keep the PR's own stated acceptance gaps explicit blockers for claiming #707 live-migration acceptance: green fork workflows on this exact head, matching-runtime BOT CLI/MCP verification, and at least one genuine historical pre-paging fixture peak-RSS measurement. The synthetic 'remove pages from current writer' fixture proves descriptor compatibility, not historical memory behavior.
The 15-minute corpus authority is now qualitatively correct (finite + lifecycle cancellation), but the acceptance should record the measurement that justifies 15 minutes rather than only asserting the constant in a unit test. A constant-equality test protects configuration, not the cost model.
Review finding on ScriptedAlchemy#882: `branch_add` polled the scheduler serving slot and dashboard freshness every 10ms (`BRANCH_GENERATION_SEATING_POLL_INTERVAL`) for up to the 30 minute hard deadline, ~180k iterations per waiter. Root cause: the scheduler broadcasts a generation publication when reconcile seals, which is before the sealed generation takes the serving slot. The waiter needs the seated slot, and there is no second publication after the seat, so the only way it could observe the transition was to poll for it. The scheduler now advances a `watch` seating counter after it writes the serving slot -- in both seat paths, the reconcile serving swap and the ignored-dependency swap -- and `branch_add` awaits that transition alongside the publication stream. The select sleeps to the nearer of the idle and hard deadlines, so both bounds still fire and neither is polled for. Also fixes two pre-existing clippy `-D warnings` failures in the same file (`items_after_test_module`, `duration_suboptimal_units`) that block the merge independently. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Addressed the Root cause. The scheduler broadcasts a generation publication when reconcile seals ( Fix.
I reused the existing publication broadcast rather than adding a second one — it fires too early for this waiter by design, so a distinct seating signal was needed. No existing notify/watch covered seating ( Evidence. New test 0.37s against the 10ms×N poll it replaces, with no poll interval left in the loop. New test
|
Validation on head c6130cb (2026-09-05)
Not merging yet. Next: stream legacy evidence without the DOM so restore RSS stays bounded, keep the RSS harness as a real bounded test, then re-run the semantic activation probe. |
|
Semantic activation probe (run 3, after waiting for index readiness): |
Restoring a pre-paging generation read its whole evidence segment, parsed
a `serde_json::Value` from it, rewrote identity markers in that tree, then
deserialized the tree again. Peak RSS was the segment plus a DOM plus the
typed payload, and it grew with the corpus: 2.348x the on-disk generation
at 500 files, 2.370x at 2000.
The segment is now read through the same bounded page reader the paged
form uses -- a pre-paging segment carries no page table, so it is ranged
in 256 KiB chunks and still authenticated by its aggregate digest -- and
the identity markers are restored by a `serde` transcoder as the stream
is deserialized. The classification rules are the replaced DOM walk's,
unchanged: a string is substituted by the object key that encloses it,
reset at every object member and inherited through arrays; an invalid key
still fails with its original contract message. Verification takes the
same reader, so it no longer materializes a legacy segment either.
Measured as peak-RSS growth over a baseline warmed by a paged restore of
the same generation, which isolates the pre-paging path's own cost:
files generation evidence before after
300 12,750,156 5,402,171 1.279x (15.9M) 0.232x (2.9M)
1500 63,747,800 27,005,771 1.272x (79.2M) 0.168x (10.5M)
The measurement harness is now a bounded test rather than a probe: it
builds a 300-file generation, rewrites its descriptor into the pre-paging
shape, and fails if the legacy restore's peak growth reaches half the
on-disk generation size. It takes its reading in a child process because
VmHWM is process-wide, and returns early where /proc/self/status and
clear_refs are unavailable. The pre-paging descriptor test now asserts
the bounded read shape it produces: no whole-segment read, and no single
read larger than one page.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…plan-reopened' into fable/882-seating-notify # Conflicts: # crates/tracedecay-code-index-runtime/src/code_index_scheduler/registry.rs
|
Legacy restore blocker addressed in Root cause. Fix. Two changes, both reusing what the paged form already does:
Numbers. Measured as peak RSS (VmHWM) growth over a baseline warmed by a paged restore of the same generation. Warming isolates the pre-paging path's own cost: the restored generation's own footprint (~1.2x the on-disk size, unavoidable —
The ratio now shrinks as the corpus grows instead of holding constant, which is the linearity the review flagged. Absolute peak for a full restore at 500 files went from 47.6 MB to the paged form's own ~24 MB. Test. The measurement harness is a real bounded test now, not an
Verification. Merge. |
|
Merging into codex/tracedecay-total-redesign-plan-reopened per owner decision (#707 is the long-lived integration branch). Validation closed: branch_add seating is watch-driven (c6130cb; serving_seat_wake_arrives_only_after_the_slot_is_seated, the_hard_deadline_bounds_every_seating_wake); legacy pre-paging restore streams the evidence segment (4f90915; 0.23× generation at 300 files, 0.17× at 1500, was 1.28×/2.7×), bounded test not ignored; workspace clippy clean; CLI and MCP report the same exact generation on an isolated daemon. The semantic activation defect found by that probe (#905) is fixed on the base branch (c5fe03731…4e6ecf029) and is inherited by this merge. |
5ee2b4c
into
ScriptedAlchemy:codex/tracedecay-total-redesign-plan-reopened
Summary
Retained pre-paging evidence manifests can now be authenticated and restored without allocating owned identity maps for every symbol and chunk in the corpus. Encountered legacy markers resolve directly against canonical descriptors and decoded files. The full manifest parser and streaming retention reader share bounded layout validation while retaining separate authentication responsibilities; missing
pagesidentifies historical format, and explicitnullis rejected.Semantic generation restart and publication use fresh, finite 15-minute background authority per phase. Tests cover the exact budget and lifecycle cancellation during snapshot refresh. The existing lexical cursor preservation and adaptive subdivision of refused page batches remain included.
Validation of the follow-up fixes
74da6279f(codec) and316e8e73f(semantic authority).Earlier live evidence
The earlier revision resumed a retained generation, completed 3,863 source files, and remounted the verified generation after restart without replay. This is historical evidence; matching-runtime and BOT CLI/MCP verification of the follow-up revision is still in progress.
Acceptance gaps
Follow-up validation for #707 and #792; shared validation addresses #886. Related: #753 and #837.