fix(retention): defer a census that races a missing path - #1811
Merged
Merged
Conversation
A generation-retention census that does not hold the store lock can observe a scope root the publisher has not created, or a file a peer unlinked after it was listed. That NotFound is the same deferral as a held writer, not a storage failure. The mounted journey waits until the superseded source is actually collectable. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
|
Opening a listed generation that publication already unlinked must not be Storage. The census defers as store-busy; a non-NotFound open stays storage. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
ScriptedAlchemy
marked this pull request as ready for review
September 19, 2026 07:01
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1518364577
ℹ️ 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".
| is_cancelled: &dyn Fn() -> bool, | ||
| ) -> Result<u32, CodeGenerationRetentionErrorV1> { | ||
| let mut file = File::open(path).map_err(storage)?; | ||
| let mut file = File::open(path).map_err(deferred_if_absent)?; |
There was a problem hiding this comment.
When the vanished entry comes from graph_replay_pool_root, sweep_unreferenced_generation_segments calls this function while scanning that pool, but the shared mapper always returns GenerationStoreBusy. The maintenance caller handles that as generation_store_busy, whereas GraphReplayPoolBusy records the replay failure and arms its bounded backoff, so a concurrent replay retirement is misreported and can be retried loudly every maintenance tick. Pass the scan context through and classify missing replay-pool entries as GraphReplayPoolBusy (including the subsequent changed File::open).
AGENTS.md reference: AGENTS.md:L152-L154
Useful? React with 👍 / 👎.
…n-torn-census-c34f # Conflicts: # crates/tracedecay/src/daemon/production_harness/generation_retention_test.rs
The merged branch reclassified every `NotFound` in the generation census as `GenerationStoreBusy`. Two of those sites are not publisher races: - The generations directory read under a live active pointer. The sealer writes the sealed file, its directory and the pointer under one store-lock hold, so a durable pointer implies a durable directory; an absent directory beside a live pointer is loss. Production consumes `GenerationStoreBusy` with `defer_generation_store_busy`, so deferring here turns that loss into a silent per-tick defer that never reclaims and never reports degraded. - `read_active_pointer`. Its lock-free caller `read_optional_active_pointer` already stats the pointer and owns the typed unpublished answer, and the pointer is installed by atomic rename so it is never transiently absent; its other caller, `mutate_verified_text_artifact_under_lock`, holds the store lock and compares against an expected pointer. The enumerate-then-open sites keep the deferral: the census lists a directory without the store lock, so a name it just read can be unlinked before the open, which is the race the branch set out to fix. Drops the unit test that asserted the reverted classification. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
neoneye
pushed a commit
to agent-memory-atlas-archive/ScriptedAlchemy--tracedecay
that referenced
this pull request
Sep 22, 2026
…ling-3ef6) Stamps a worker continuation while `reconcile_in_progress` still reports the pass, so a seat waiter that sees the counter reach zero cannot sample an empty slot and then race the failure ceiling. Treats a scoped store directory that cold open has not created yet as an unpublished plan rather than `Storage(NotFound)`. Conflict resolutions against the scheduler work batch C already carries from ScriptedAlchemy#1826, ScriptedAlchemy#1821 and ScriptedAlchemy#1811: - `code_index_scheduler/registry.rs`: identical `note_visible_worker_continuation` body on both sides, only the doc prose differed. Kept batch C's landed wording; rewording a landed comment is churn with no behavior change. - `code_index_scheduler/registry/mount.rs`: six hunks. Four were comment wording for identical code. The other two are the same stamp at different positions: batch C stamps the retained-recovery successor before the recovery await, and the roster-refusal rebuild right after `take_ignored_roster_refusal_rebuild` through the `prepare_*` handles, while this PR stamps both later and re-enters a pass guard to do it. Kept batch C's earlier placement, which holds the guard across a strictly wider window and so satisfies the invariant this PR is after. No production line of this PR is lost by that choice. - `code_index_generations/tests.rs`: both sides added an independent test at the same offset. Kept both, batch C's `vanished_listed_generation_open_defers_instead_of_storage_loss` and this PR's `missing_store_is_an_unpublished_plan_not_a_storage_failure`. - `production_harness/generation_retention_test.rs`: took the union. This PR's canonical project root, serving-seat wake and convergence predicate all stay, because the predicate waits for the superseded source to actually be collectable instead of accepting the first plan the planner returns. Batch C's `GraphReplayPoolBusy` arm is folded into the deferral match, so that typed answer is still consumed rather than panicking the test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
neoneye
pushed a commit
to agent-memory-atlas-archive/ScriptedAlchemy--tracedecay
that referenced
this pull request
Sep 22, 2026
Two landed changes disagreed about a store root that does not exist yet. ScriptedAlchemy#1811 deferred with `GenerationStoreBusy`, ScriptedAlchemy#1836 returned the unpublished plan. Both tests survived the batch-D merge, so `preparation_defers_when_the_scope_root_does_not_exist_yet` failed deterministically against ScriptedAlchemy#1836's early return. Keep the unpublished plan. The only production caller is `tracedecay-maintenance::store_maintenance::run_code_generation_retention`. It routes `GenerationStoreBusy` through `defer_generation_store_busy`, which logs `retention_degraded failure=generation_store_busy` and returns `Failed`. `generation.rs` turns `Failed` into `MaintenanceTickOutcome` `Retry`, which `tick.rs` schedules on the short `retry_delay` and counts in `daemon.maintenance.generation.retry_total`. That deferral is a counted, logged failure, not a quiet wait. An empty plan returns `Complete`, which `tick.rs` schedules on the ordinary `interval`. Neither answer is terminal, so no caller plans once and stops. The maintenance loop re-plans every tick, and the mounted retention journey loops on `Ok(_)` and `GenerationStoreBusy` alike. The difference is duration. An absent scope root has no publisher to wait for, because cold open creates it inside the worker, so a mounted project that is never indexed keeps an absent root for the life of the daemon and the deferral would report degraded on every tick. The unpublished plan still converges the moment the store appears. The failure ceiling ScriptedAlchemy#1836 names is `SERVING_SEAT_FAILURE_CEILING`, a test-only wall bound on a positive serving-seat wait in `code_index_scheduler/tests/mod.rs`. It consumes no retention error, so it constrains neither contract. ScriptedAlchemy#1811's deeper `NotFound` mapping stays. `deferred_if_absent` still covers the two enumerate-then-open sites, where `read_dir` already proved the name existed and a peer can unlink it before the open. That race is unreachable from the root check, and d6d8665 already reverted the two durable-state sites on the same reasoning. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GenerationStoreBusywhen the scope root or a just-listed file is missing, instead of a storage failure.NotFoundopen failure stays storage (vanished_listed_generation_open_defers_instead_of_storage_loss).Motivation
Tip
3f71bccb2d(merge of #1796) failedTest Linux root-libonmounted_code_generation_retention_continues_capped_segment_reclamation. Try 1 panicked withStorage("No such file or directory (os error 2)")at the retention plan; try 2 passed, and nextest still fails the job. The census does not hold the store lock, so it can observe a scope root the publisher has not created yet, or a file a peer unlinked afterread_dir. That is the same deferral as a held writer.Did not touch #1794 (
cursor/fix-master-tip-ci-races-0baa); that branch stays with its owner. Did not restorepull_requestinci.yml.Changes
code_index_generations:NotFoundon the census and lock-open paths isGenerationStoreBusy. Other I/O errors stayStorage.generation_retention_test: poll until the superseded generation is collectable, retrying store-busy and replay-pool-busy.tests.rs: the open of a vanished listed generation is the deferral, notStorage.Test plan
cargo test -p tracedecay-code-index-retention --lib preparation_— 6 passed, including the two new deferral casesvanished_listed_generation_open_defers_instead_of_storage_loss— 1 passedcargo test -p tracedecay --lib daemon::production_harness::generation_retention_test::mounted_code_generation_retention_continues_capped_segment_reclamation— 1 passed in 2.92sChecklist
.envfiles includedCHANGELOG.mdupdated (test and typed-error classification only; no user-facing change)workflow_dispatch SHA:
151836457716b30cde4c7009f13d081ed2c49022