diff --git a/docs/contextwiki-core-understanding.md b/docs/contextwiki-core-understanding.md index 22700fb..565d173 100644 --- a/docs/contextwiki-core-understanding.md +++ b/docs/contextwiki-core-understanding.md @@ -113,7 +113,13 @@ sync_all() This fans out one concurrent `sync_source()` run per retained source. Each source still keeps its own SQLite running-job guard, so a source that is already syncing is reported as blocked in the aggregate result instead of -starting a second overlapping fetch. +starting a second overlapping fetch. The running-job guard also refreshes a +sync-owner heartbeat beside the per-job heartbeat during active running-job +metadata updates. When a previous owner still looks alive only because both old +and new containers report the app as PID `1`, startup recovery and +`begin_sync_job()` specifically fall back to the running job's own staleness +window before reclaiming the source, instead of treating the short unowned-job +grace as proof that the previous sync is dead. Obsidian example: diff --git a/docs/plan/2026-06-15-notion-docker-owner-recovery.md b/docs/plan/2026-06-15-notion-docker-owner-recovery.md new file mode 100644 index 0000000..3aedcc4 --- /dev/null +++ b/docs/plan/2026-06-15-notion-docker-owner-recovery.md @@ -0,0 +1,109 @@ +## User request + +- Fix the stuck Notion sync problem caused while running the MCP server from Claude Desktop with Docker. +- Do both: + - recover the current stuck local sync state + - patch the code so Docker restarts do not keep stale running jobs alive +- Clarify whether two running Docker containers mean one was started by Claude and whether the user still needs to run one manually. + +## Branch preflight result + +- Date: `2026-06-15` +- Starting worktree: `/Users/eunhwa/.codex/worktrees/53e0/MCPContentSearch` +- Initial state: clean detached HEAD at `25ce3b5` +- Freshness check: `git fetch origin main` succeeded +- Task branch: `feature/notion-docker-owner-recovery` created from `origin/main` +- Safety: target edits will not happen on `main` + +## Scope + +- Add regression coverage for Docker-style sync owner persistence where multiple container generations record `process_id=1`. +- Patch `storage/metadata_store.py` so startup and runtime recovery can distinguish the current owner from stale previous owners even when PID liveness checks are not enough in containers. +- Safely mark the currently stuck local `running` sync jobs as failed in SQLite metadata only, without touching user document/chunk content or Chroma vectors. +- Confirm that no leftover `running` sync rows remain in metadata after the + bounded local repair. + +## Non-goals + +- Do not change MCP tool contracts. +- Do not inspect or mutate user document/chunk contents. +- Do not delete or reset local Chroma data. +- Do not change Notion connector request logic unless testing proves it is needed. + +## Acceptance criteria + +- A failing test first demonstrates that a previous-owner running job can be recovered even when the stored PID remains alive in a Docker-style `process_id=1` scenario. +- `MetadataStore.recover_orphaned_running_jobs()` and/or follow-on `begin_sync_job()` no longer treat stale previous-owner rows as live solely because PID `1` exists in the new container. +- Focused storage tests pass. +- Syntax verification for repo Python modules passes. +- The bounded local metadata repair only changes sync-job/source status fields. +- After the metadata repair, the post-repair readback shows no leftover + `running` sync rows in local sync metadata. + +## Step breakdown + +1. `storage-owner-regression` + - Read current `MetadataStore` owner recovery flow and nearby tests. + - Add a regression that models Docker container restart behavior with old and new owners both using PID `1`. + - Verify the new test fails before implementation. +2. `storage-owner-fix` + - Patch owner liveness/recovery logic in `storage/metadata_store.py` with the smallest boundary that preserves current non-Docker behavior. + - Keep the fix inside storage metadata ownership rules; do not spread Docker-specific logic into API/fetching layers. +3. `focused-verification` + - Run the targeted storage tests. + - Run compile verification. +4. `local-metadata-repair` + - Update only stuck local sync job/source status rows to a recovery failure message. + - Re-read the rows to confirm the post-repair metadata state. + +## Files likely to change + +- `storage/metadata_store.py` +- `tests/storage/test_metadata_store.py` +- `docs/contextwiki-core-understanding.md` +- `docs/plan/2026-06-15-notion-docker-owner-recovery.md` + +## Test and verification plan + +- Red: targeted failing storage regression + - `uv run --locked pytest -q tests/storage/test_metadata_store.py -k "docker or previous_owner"` +- Green: + - `uv run --locked pytest -q tests/storage/test_metadata_store.py` +- Syntax: + - `python -m compileall api core environments fetching indexing search storage main.py` + +## Functional smoke matrix plan + +| Surface | Scenario | Safe mode | Planned status | +| --- | --- | --- | --- | +| SQLite sync metadata | Recover stale Docker-owner running job | Temp test DB | passed | +| Local metadata cleanup | Confirm post-repair metadata no longer contains leftover `running` sync rows | Existing local metadata only; no content reads | passed | +| Functional E2E gate | Retained MCP caller-surface smoke via repo script | Local deterministic suite | passed | + +## Architecture/ADR constraints + +- Keep the fix in `storage/metadata_store.py` per ADR 0001 layered boundaries. +- Preserve SQLite as the operational source of truth beside Chroma per ADR 0002. +- Do not broaden MCP scope or change retained tool contracts per ADR 0006. +- Raw secrets must not be logged or persisted. + +## Risks and rollback notes + +- Risk: over-recovering a genuinely live sync job. + - Mitigation: make the recovery rule narrow to previous-owner rows that cannot be the current store owner, and cover it with regression tests. +- Risk: local metadata repair could touch user data. + - Mitigation: limit the repair to `sync_jobs` and `sources` status/error fields only; no document/chunk/chroma mutation. +- Rollback: + - Revert the code diff. + - If the local metadata repair proves wrong, statuses can be re-created by rerunning syncs; no indexed content rows or vectors are deleted in this plan. + +## Progress log + +| Phase | Status | Summary | Evidence | +| --- | --- | --- | --- | +| Branch preflight | completed | Verified clean detached worktree, fetched `origin/main`, created `feature/notion-docker-owner-recovery`. | `git status --short --branch`; `git fetch origin main`; `git switch -c feature/notion-docker-owner-recovery origin/main` | +| Planning | completed | Wrote plan for storage-owner fix plus bounded local metadata repair. | `docs/plan/2026-06-15-notion-docker-owner-recovery.md` | +| Regression test | completed | Added a failing regression for stale previous-owner recovery when PID liveness is a Docker false positive. | `uv run --locked pytest -q tests/storage/test_metadata_store.py -k "stale_previous_owner_even_if_pid_looks_alive"` failed with `assert 0 == 1` | +| Implementation | completed | Refreshed owner heartbeats during running-job activity, then tightened same-PID previous-owner recovery to use running-job staleness instead of the short unowned grace after review findings. | `storage/metadata_store.py`; `tests/storage/test_metadata_store.py` | +| Focused verification | completed | Re-ran targeted storage tests, full storage test file, compile verification, and functional E2E after the review-driven fix. | `uv run --locked pytest -q tests/storage/test_metadata_store.py -k "same_pid_previous_owner or stale_previous_owner_even_if_pid_looks_alive or live_previous_owner or previous_owner or begin_sync_job_preserves_same_pid_previous_owner or begin_sync_job_recovers_stale_same_pid_previous_owner"`; `uv run --locked pytest -q tests/storage/test_metadata_store.py`; `python -m compileall api core environments fetching indexing search storage main.py`; `./scripts/verify_functional_e2e.sh` | +| Local metadata repair | completed | Confirmed the post-repair metadata readback had no remaining `running` sync rows. | readback query showed zero remaining `sync_jobs.status = running` rows | diff --git a/storage/metadata_store.py b/storage/metadata_store.py index f52f060..a723a79 100644 --- a/storage/metadata_store.py +++ b/storage/metadata_store.py @@ -457,6 +457,7 @@ def touch_sync_job(self, job_id: str) -> Optional[SyncJobModel]: """, (heartbeat_at, job_id, SyncJobStatus.RUNNING.value), ) + self._touch_sync_owner(conn, heartbeat_at) row = conn.execute("SELECT * FROM sync_jobs WHERE job_id = ?", (job_id,)).fetchone() return self._job_from_row(row) if row else None @@ -507,6 +508,7 @@ def validate_running_job_document(self, job_id: str, document: DocumentModel) -> """, (heartbeat_at, job_id), ) + self._touch_sync_owner(conn, heartbeat_at) row = conn.execute("SELECT * FROM sync_jobs WHERE job_id = ?", (job_id,)).fetchone() return self._job_from_row(row) @@ -562,6 +564,7 @@ def upsert_document_and_replace_chunks_for_running_job( """, (heartbeat_at, job_id), ) + self._touch_sync_owner(conn, heartbeat_at) self._upsert_document(conn, normalized) self._record_chunk_tombstones_for_document(conn, document_id, normalized.source_id) conn.execute( @@ -1410,7 +1413,11 @@ def _should_fail_active_running_job(self, conn, row) -> bool: ).fetchone() if not owner_row: return False - return not self._is_process_alive(owner_row["process_id"]) + if not self._is_process_alive(owner_row["process_id"]): + return True + if self._owner_pid_matches_current_process(owner_row): + return self._is_stale_running_job(row) + return False def _should_recover_startup_running_job(self, conn, row) -> bool: owner_id = row["owner_id"] if "owner_id" in row.keys() else "" @@ -1422,7 +1429,11 @@ def _should_recover_startup_running_job(self, conn, row) -> bool: if owner_row: if owner_id == self.sync_owner_id: return self._is_stale_running_job(row) - return not self._is_process_alive(owner_row["process_id"]) + if not self._is_process_alive(owner_row["process_id"]): + return True + if self._owner_pid_matches_current_process(owner_row): + return self._is_stale_running_job(row) + return False return self._is_stale_running_job(row) return self._should_recover_unowned_running_job(row) @@ -1437,6 +1448,13 @@ def _should_recover_unowned_running_job(self, row) -> bool: seconds=self.unowned_running_job_grace_seconds ) + @staticmethod + def _owner_pid_matches_current_process(row) -> bool: + try: + return int(row["process_id"]) == os.getpid() + except (TypeError, ValueError): + return False + def _touch_sync_owner(self, conn, timestamp: str): process_id = os.getpid() conn.execute( diff --git a/tests/storage/test_metadata_store.py b/tests/storage/test_metadata_store.py index 86d0e2e..02b867e 100644 --- a/tests/storage/test_metadata_store.py +++ b/tests/storage/test_metadata_store.py @@ -47,6 +47,27 @@ def _mark_job_running( return store.get_sync_job(job_id) +def _mark_owner_heartbeat( + store: MetadataStore, + owner_id: str, + *, + started_at: str | None = None, + heartbeat_at: str | None = None, +): + started_at = started_at or datetime.now(timezone.utc).isoformat() + heartbeat_at = heartbeat_at or started_at + with store._connect() as conn: + conn.execute( + """ + UPDATE sync_job_owners SET + started_at = ?, + heartbeat_at = ? + WHERE owner_id = ? + """, + (started_at, heartbeat_at, owner_id), + ) + + def _insert_legacy_web_source_row(store: MetadataStore): now = datetime.now(timezone.utc).isoformat() store.ensure_schema() @@ -605,6 +626,206 @@ def test_recover_orphaned_running_jobs_recovers_dead_previous_owner(tmp_path, mo assert fresh.job_id != previous_job.job_id +def test_recover_orphaned_running_jobs_recovers_stale_previous_owner_even_if_pid_looks_alive( + tmp_path, + monkeypatch, +): + store = MetadataStore( + tmp_path / "contextwiki.sqlite3", + running_job_timeout_seconds=24 * 60 * 60, + sync_owner_id="current-owner", + ) + previous_store = MetadataStore( + store.db_path, + running_job_timeout_seconds=24 * 60 * 60, + sync_owner_id="previous-owner", + ) + store.upsert_source( + SourceModel( + source_id="source_github", + source_type=SourceType.GITHUB, + name="GitHub", + enabled=True, + sync_status=SyncStatus.RUNNING, + ) + ) + previous_job, started = previous_store.begin_sync_job("source_github") + assert started is True + stale_owner_heartbeat = "2026-06-01T00:00:00+00:00" + _mark_job_running( + store, + previous_job.job_id, + started_at=stale_owner_heartbeat, + heartbeat_at=stale_owner_heartbeat, + ) + _mark_owner_heartbeat( + store, + "previous-owner", + started_at=stale_owner_heartbeat, + heartbeat_at=stale_owner_heartbeat, + ) + monkeypatch.setattr(MetadataStore, "_is_process_alive", staticmethod(lambda process_id: True)) + + recovered_count = store.recover_orphaned_running_jobs( + started_before="2026-06-02T00:00:00+00:00", + error_message="Previous running sync job was recovered after server restart; start sync again.", + ) + fresh, fresh_started = store.begin_sync_job("source_github") + + assert recovered_count == 1 + assert store.get_sync_job(previous_job.job_id).status == SyncJobStatus.FAILED + assert fresh_started is True + assert fresh.job_id != previous_job.job_id + + +def test_recover_orphaned_running_jobs_preserves_same_pid_previous_owner_when_job_heartbeat_is_fresh( + tmp_path, + monkeypatch, +): + store = MetadataStore( + tmp_path / "contextwiki.sqlite3", + running_job_timeout_seconds=24 * 60 * 60, + sync_owner_id="current-owner", + ) + previous_store = MetadataStore( + store.db_path, + running_job_timeout_seconds=24 * 60 * 60, + sync_owner_id="previous-owner", + ) + store.upsert_source( + SourceModel( + source_id="source_github", + source_type=SourceType.GITHUB, + name="GitHub", + enabled=True, + sync_status=SyncStatus.RUNNING, + ) + ) + previous_job, started = previous_store.begin_sync_job("source_github") + assert started is True + fresh_job_heartbeat = datetime.now(timezone.utc).isoformat() + stale_owner_heartbeat = "2026-06-01T00:00:00+00:00" + _mark_job_running( + store, + previous_job.job_id, + started_at="2026-06-01T00:00:00+00:00", + heartbeat_at=fresh_job_heartbeat, + ) + _mark_owner_heartbeat( + store, + "previous-owner", + started_at=stale_owner_heartbeat, + heartbeat_at=stale_owner_heartbeat, + ) + monkeypatch.setattr(MetadataStore, "_is_process_alive", staticmethod(lambda process_id: True)) + + recovered_count = store.recover_orphaned_running_jobs( + started_before="2026-06-02T00:00:00+00:00", + error_message="Previous running sync job was recovered after server restart; start sync again.", + ) + returned, started = store.begin_sync_job("source_github") + + assert recovered_count == 0 + assert store.get_sync_job(previous_job.job_id).status == SyncJobStatus.RUNNING + assert started is False + assert returned.job_id == previous_job.job_id + + +def test_begin_sync_job_preserves_same_pid_previous_owner_when_job_heartbeat_is_fresh( + tmp_path, + monkeypatch, +): + store = MetadataStore( + tmp_path / "contextwiki.sqlite3", + running_job_timeout_seconds=24 * 60 * 60, + sync_owner_id="current-owner", + ) + previous_store = MetadataStore( + store.db_path, + running_job_timeout_seconds=24 * 60 * 60, + sync_owner_id="previous-owner", + ) + store.upsert_source( + SourceModel( + source_id="source_github", + source_type=SourceType.GITHUB, + name="GitHub", + enabled=True, + sync_status=SyncStatus.RUNNING, + ) + ) + previous_job, started = previous_store.begin_sync_job("source_github") + assert started is True + fresh_job_heartbeat = datetime.now(timezone.utc).isoformat() + stale_owner_heartbeat = "2026-06-01T00:00:00+00:00" + _mark_job_running( + store, + previous_job.job_id, + started_at="2026-06-01T00:00:00+00:00", + heartbeat_at=fresh_job_heartbeat, + ) + _mark_owner_heartbeat( + store, + "previous-owner", + started_at=stale_owner_heartbeat, + heartbeat_at=stale_owner_heartbeat, + ) + monkeypatch.setattr(MetadataStore, "_is_process_alive", staticmethod(lambda process_id: True)) + + returned, started = store.begin_sync_job("source_github") + + assert started is False + assert returned.job_id == previous_job.job_id + assert store.get_sync_job(previous_job.job_id).status == SyncJobStatus.RUNNING + + +def test_begin_sync_job_recovers_stale_same_pid_previous_owner_and_starts_fresh_job( + tmp_path, + monkeypatch, +): + store = MetadataStore( + tmp_path / "contextwiki.sqlite3", + running_job_timeout_seconds=24 * 60 * 60, + sync_owner_id="current-owner", + ) + previous_store = MetadataStore( + store.db_path, + running_job_timeout_seconds=24 * 60 * 60, + sync_owner_id="previous-owner", + ) + store.upsert_source( + SourceModel( + source_id="source_github", + source_type=SourceType.GITHUB, + name="GitHub", + enabled=True, + sync_status=SyncStatus.RUNNING, + ) + ) + previous_job, started = previous_store.begin_sync_job("source_github") + assert started is True + stale_timestamp = "2026-06-01T00:00:00+00:00" + _mark_job_running( + store, + previous_job.job_id, + started_at=stale_timestamp, + heartbeat_at=stale_timestamp, + ) + _mark_owner_heartbeat( + store, + "previous-owner", + started_at=stale_timestamp, + heartbeat_at=stale_timestamp, + ) + monkeypatch.setattr(MetadataStore, "_is_process_alive", staticmethod(lambda process_id: True)) + + fresh, fresh_started = store.begin_sync_job("source_github") + + assert fresh_started is True + assert fresh.job_id != previous_job.job_id + assert store.get_sync_job(previous_job.job_id).status == SyncJobStatus.FAILED + + def test_recover_orphaned_running_jobs_preserves_live_previous_owner(tmp_path, monkeypatch): store = MetadataStore( tmp_path / "contextwiki.sqlite3",