fix(index): wire the panic guard into the reconcile worker - #717
fix(index): wire the panic guard into the reconcile worker#717ScriptedAlchemy wants to merge 3 commits into
Conversation
A real repository could never finish indexing: an empty-string test.describe suite name in react-router hit a debug_assert in generate_node_id, panicked the index worker, and the scheduler retried the same unit 114 times. The assert also compiled out in release, so release builds silently minted a node id for an empty name instead — the two profiles disagreed on the same input. Source text is input, not a programmer-contract violation, so generate_node_id no longer asserts on it, and a panicking reconcile unit is now isolated rather than poisoning the generation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The earlier quarantine was reverted as ineffective, and correctly so: ReconcilePanicGuardV1 was declared but never reached from the worker, so a panicking unit was still retried on every wake. This connects record_panic, suppresses_pass and record_progress to the registry worker and covers it with an isolation test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…plan-reopened' into HEAD
|
![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: ecc5a5997d
ℹ️ 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".
| if panic_guard.suppresses_pass( | ||
| tokio::time::Instant::now(), | ||
| worker_control_epoch.load(Ordering::Acquire), | ||
| ) { |
There was a problem hiding this comment.
After the panic limit is reached, this suppression checks only worker_control_epoch, but the query freshness path detects Git metadata changes and raw-file staleness through request_fresh_for_query_background(), whose request_background_reconcile() sets overflow and wakes the worker without advancing that epoch. Consequently, fixing the offending file via checkout, rsync, or an editor without an after-edit hook still leaves every query-driven wake suppressed until another hook event or daemon restart.
Useful? React with 👍 / 👎.
| let decision = panic_guard.record_panic( | ||
| tokio::time::Instant::now(), | ||
| worker_control_epoch.load(Ordering::Acquire), | ||
| ); |
There was a problem hiding this comment.
The epoch is loaded only after the blocking reconcile has panicked. If a source-change hint advances the epoch while the fourth attempt is already processing the old snapshot, that attempt records the new epoch as quarantined; the pending wake for the changed source then observes the same epoch and is suppressed, stranding the fix until another event arrives. Capture the attempt epoch before dispatch and pass that value to record_panic.
Useful? React with 👍 / 👎.
| #[cfg(test)] | ||
| if let Some(fault) = self.reconcile_fault.clone() { | ||
| fault.arrive()?; |
There was a problem hiding this comment.
This production execution path now branches through a #[cfg(test)] fault-injection field and setter, allowing the worker tests to fabricate panic, capacity, and identity failures before the real source-indexing and resident-memory authorities run. Those tests can therefore remain green while the production failure paths drift, and the repository explicitly prohibits test-only production ports; drive the actual failure boundaries instead.
AGENTS.md reference: AGENTS.md:L82-L84
Useful? React with 👍 / 👎.
| // Permanent refusals deliberately never reach | ||
| // here: retrying those forever is the failure | ||
| // this loop already had. | ||
| match capacity_retry.record_capacity_failure() { |
There was a problem hiding this comment.
Once a contention episode exhausts capacity_retry, only a successful or non-capacity pass resets its counter. A later real edit gets one externally driven attempt, but if capacity happens to be occupied during that attempt, this call immediately returns None; releasing capacity emits no wake, so the new source remains stale until another unrelated event. Reset the retry episode when new input arrives, as the policy documentation claims.
Useful? React with 👍 / 👎.
|
Closing after selective reconciliation for PR 707. Retained only the graph.rs behavior that accepts empty extractor-derived names and its two deterministic identity tests. RED reproduced both debug-assert panics; GREEN passed 2 of 2 in isolated target /tmp/tracedecay-empty-name-tdd.SbLUtK. The reviewed panic and capacity machinery was not taken. |
1 participant
Reopened against
codex/tracedecay-total-redesign-plan-reopened.This answers the "ineffective panic quarantine" revert directly. That revert was right:
ReconcilePanicGuardV1was declared but never reached from the worker, so a panicking unit was still retried on every wake — the original symptom was the same panic repeating 114 times.This connects
record_panic,suppresses_passandrecord_progressto the registry worker, with a 371-line isolation test. Re-proposing reverted work is the owner's call — close this if you disagree with the direction rather than the implementation.