fix(code-index): restore watcher events after restart

[WebMad]

Summary

Refs #1819; second narrow split from #1821, independent of #1834 and based directly on main.

• Restore batch emitters after disposal and reinitialization.
• Avoid duplicate native watchers and reset watcher/timer references on disposal.
• Capture per-session emitters so old batches cannot notify subscribers of a restarted session.
• Include concrete watcher tests and accurate disposed-emitter mocks; existing test ESLint suppressions decrease from 25 to 22.

Scope decision / rollback

At the author's request, commit bae7be8 reverts the additional active-batch draining work from 770f0eb. The watcher implementation matches #1821 at 27fdc41. The idle/drain API and follow-up tests were removed. Original PR #1835 test improvements remain.

This PR isolates notifications only. It does not await or cancel active persistence writes before restart or index clearing. The Persistence Integrity finding about overlapping old/new writes remains UNRESOLVED. This rollback is a scope decision, not a persistence-safety fix. No additional regression fixes or orchestrator changes are included.

Validation after rollback

• Complete code-index suite: 685 tests passed across 30 files.
• Changed-file ESLint with suppression pruning and TypeScript no-emit checks passed.
• Commit/push lint and type hooks and diff whitespace checks passed.
• Local Node 24.7.0 differs from requested 22.23.1; CI still needs verification.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: Zoo-Code-Org/Zoo-Code/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 99bd833b-d56d-4918-8714-51ed460aba8c

📥 Commits

Reviewing files that changed from the base of the PR and between d351a15 and 16e5bcd.

📒 Files selected for processing (3)

• src/eslint-suppressions.json
• src/services/code-index/processors/__tests__/file-watcher.spec.ts
• src/services/code-index/processors/file-watcher.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

📜 Recent review details

🧰 Additional context used

📓 Path-based instructions (5)

Check persistence and lifecycle invariants: awaited atomic writes, rollback or explicit partial-failure behavior, cross-window state consistency, stale listeners/watchers, cancellation, idempotency, and safe restart/resume without lost or d...

⚙️ CodeRabbit configuration file

Files:

• src/services/code-index/processors/__tests__/file-watcher.spec.ts
• src/services/code-index/processors/file-watcher.ts

Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases.

⚙️ CodeRabbit configuration file

Files:

• src/services/code-index/processors/__tests__/file-watcher.spec.ts

Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths.

⚙️ CodeRabbit configuration file

Files:

• src/services/code-index/processors/__tests__/file-watcher.spec.ts
• src/services/code-index/processors/file-watcher.ts

Verify extension/webview contracts, cancellation and error propagation, VS Code lifecycle correctness, and behavior under retries and partial failure.

⚙️ CodeRabbit configuration file

Files:

• src/eslint-suppressions.json
• src/services/code-index/processors/__tests__/file-watcher.spec.ts
• src/services/code-index/processors/file-watcher.ts

Act as an adversarial second-opinion reviewer.

⚙️ CodeRabbit configuration file

Files:

• src/eslint-suppressions.json
• src/services/code-index/processors/__tests__/file-watcher.spec.ts
• src/services/code-index/processors/file-watcher.ts

🔇 Additional comments (3)

src/services/code-index/processors/file-watcher.ts (1)

44-72: LGTM!

Also applies to: 116-126, 145-153, 205-214, 227-227, 246-246, 269-269, 287-287, 301-301, 367-367, 452-463, 492-492, 508-508, 521-531

src/services/code-index/processors/__tests__/file-watcher.spec.ts (1)

41-59: LGTM!

Also applies to: 94-126, 128-148, 150-155, 157-172, 174-204

src/eslint-suppressions.json (1)

1399-1399: LGTM!

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • Prevented stale file-processing events from appearing after a watcher restart.
  • Improved recovery after reinitialization and prevented duplicate file watchers.
  • Ensured pending work and timers are cleared on disposal.
  • Improved error reporting while preserving cached file hashes when deletion fails.

Walkthrough

FileWatcher now recreates disposed batch event emitters during initialization and clears watcher and debounce state during disposal. Each batch uses the progress and completion emitters captured at its start. Tests cover restart isolation, repeated initialization, disposal, and deletion failure.

Changes

FileWatcher lifecycle and batch events

Layer / File(s)	Summary
Emitter and watcher lifecycle src/services/code-index/processors/file-watcher.ts, src/services/code-index/processors/__tests__/file-watcher.spec.ts	Getters expose the current batch event emitters. Initialization recreates disposed emitters. Disposal resets watcher and timer state. Tests cover reinitialization, duplicate initialization, and disposal behavior.
Batch-scoped event delivery src/services/code-index/processors/file-watcher.ts, src/services/code-index/processors/__tests__/file-watcher.spec.ts, src/eslint-suppressions.json	Batch processing passes captured emitters through progress and completion notifications. Tests cover restart isolation and deletion failure outcomes. The test file’s no-explicit-any suppression count decreases from 25 to 22.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant FileWatcher
  participant BatchListeners
  participant processBatch
  FileWatcher->>BatchListeners: Fire batch start event
  FileWatcher->>processBatch: Pass captured progress and completion emitters
  processBatch->>BatchListeners: Deliver progress and completion events through captured emitters

Loading

Merge Risk: ⚪ Minimal · up to 16e5b

Watcher notifications are intended to resume after restart without sending older batch events to new subscribers. No actionable merge-blocking issue is established; complete the normal CI checks before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 16e5b

The restart change keeps notifications from an older batch separate from new subscribers. It does not appear to widen access to indexed files or add a new destination for their data. Already-running indexing work can still finish after disposal; that limitation predates this change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Filesystem events can affect the existing workspace index and its vector-store and cache writes. The changed notification routing does not establish a new file-access path or sensitive sink.

Trust Boundaries and Controls

• observed — Emitter capture occurs before start listeners run; subsequent progress and completion notifications use those captured emitters. Disposal removes the old listeners, while the orchestrator releases its old subscriptions.

Resilience and Maintainability Implications

• inferred — An old batch can still write after watcher disposal or alongside a new batch. The inspected change leaves that pre-existing index-state limitation in place rather than introducing cancellation or write-session isolation.

Hardening Proposals

• proposed — If clearing or replacing an index must exclude late writes, drain active batches or enforce write-session ownership before clearing shared index state. This is a separate safeguard, not a protection supplied by the notification change.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore (reviewers only)

❌ Failed checks (1 error, 1 inconclusive)

Check name	Status	Explanation	Resolution
Persistence Integrity	❌ Error	The restart path can overlap persistence writes. dispose() now clears fileWatcher, but it does not track or await an active triggerBatchProcessing()/processBatch() call. initialize() can the…	Drain active batches before allowing reinitialization or destructive teardown. Track the active batch promise and expose an awaitable idle operation, then make restart and index-clearing paths await it before creating a new watcher or mutat…
Linked Issues check	❓ Inconclusive	The description references issue #1819 and related pull requests, but the context does not confirm that #1819 is an approved issue or that the required issue-link format is satisfied.	Confirm that issue #1819 is approved and update the description to use the required Related GitHub Issue format, such as Closes: #1819, if applicable.

✅ Passed checks (6 passed)

Check name	Status	Explanation
Regression Evidence	✅ Passed	Focused unit coverage exists for each concrete changed behavior. file-watcher.spec.ts tests emitter restoration across all three channels, stale in-flight batch notifications after restart, duplicat…
Security Boundaries	✅ Passed	No changed path meets the security failure conditions. The implementation changes emitter lifecycle, watcher disposal, and batch event routing only. File paths still pass the existing ignored-director…
Lifecycle Resource Cleanup	✅ Passed	No changed lifecycle path leaks a resource or duplicates work. initialize() returns when fileWatcher already exists, so repeated initialization does not create another native watcher. dispose() …
Description check	✅ Passed	The description covers the implementation, scope, rollback decision, tests, and environment. It does not use the repository template headings or complete checklist, but it provides the required review…
Out of Scope Changes check	✅ Passed	The description clearly limits the change to watcher notifications and states that persistence draining, cancellation, and orchestrator changes remain out of scope. This matches the stated objectives.
Title check	✅ Passed	The title clearly and concisely describes the main change: restoring code-index watcher events after restart.

Full details: Persistence Integrity

Explanation

The restart path can overlap persistence writes. dispose() now clears fileWatcher, but it does not track or await an active triggerBatchProcessing()/processBatch() call. initialize() can therefore create a new watcher while the old batch continues. The old batch still awaits vector deletion and upsert in processBatch() and then updates cacheManager after the upsert. If a file changes from version V1 to V2 during restart, the new batch can upsert V2 and update its hash, then the old batch can upsert V1 and update the hash to V1. The vector store and cache can then represent stale or mismatched state. The base implementation did not permit reinitializing this disposed watcher because it retained the watcher reference.

Resolution

Drain active batches before allowing reinitialization or destructive teardown. Track the active batch promise and expose an awaitable idle operation, then make restart and index-clearing paths await it before creating a new watcher or mutating the vector store/cache. If draining is not possible, cancel batches before any persistence phase and add rollback or explicit partial-failure handling so an old batch cannot commit after a new session starts.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Review status

Thanks for contributing. This comment tracks the review sequence and the next action.

Current step: Required CI passed. Waiting for automated review of the latest commit.

If automated review does not start, a maintainer must restart it.

Review-state labels are managed by this workflow; do not edit them manually. community-approved is managed the same way — do not add or remove it manually. It signals a fresh community code approval for the current head as an advisory priority only; maintainer review is still required.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[coderabbitai]

Pre-merge checks failed. Please resolve the failing checks before merging.