[Fix] One reliable path for git-native document ingest & indexing - #1715
Merged
Conversation
ingest_documents committed the git revision but left Document.path and PATH_MARKER unset, deferring the pin to the async converge. For a pending/failed doc converge never runs, so the row stayed unmarked and the next connector re-sync re-authored a fresh path against the git tree, forking the file into "name (2).md" and stranding the original as an orphan. Mark each row the moment the batch lands, mirroring save_document and move_documents.
on_after_register inlined the default-workspace INSERT without setting knowledge_store_enabled, so signup workspaces defaulted to legacy even when the global switch was on -- the source of the growing unflipped backlog. Extract create_default_workspace(session, user), set the flag from config as POST /workspaces does, and cover it with a real INSERT round-trip test.
…f forking When a row loses both its PATH_MARKER and path column -- the crash window between the git commit and the mark, and the legacy unmarked-orphan rows in production -- a re-ingest re-authored the canonical name against the git tree and forked the file into "name (2).md", stranding the original. _place_unmarked now asks virtual_path_to_doc whether git already holds the canonical name for this same row (its _resolve_by_title matches an unlocated row) and, if so, reuses that path to overwrite in place. A fresh name is authored only when the canonical name is free or owned by a different row, so intra-batch distinctness is preserved.
…n't sink the batch index() rolls the shared session back on failure, which discarded every upsert in the same converge run and could crash it with MissingGreenlet. Run each document's index() in its own session; a failure rolls back only that connection.
…urable column Indexer ownership (load_owned/prune/delete_row), path->doc resolution (virtual_path_to_doc), and path display (virtual_path_of) now read Document.path instead of the document_metadata virtual_path marker. Writers still dual-write both, and recorded_virtual_path stays marker-first, so a legacy row whose column is still NULL keeps resolving until the backfill populates it. Folds the two private recorded-path readers into one shared helper.
…l_path marker Ownership and resolution now key on documents.path, but rows written before the column existed carry the path only on their metadata marker. Migration 189 copies it into the column, guarded to fill only an empty column from a /documents marker so an authored path is never overwritten. Integration test proves a legacy moved note flips from unresolvable to column-resolvable across the real resolver.
Writers now record a document's location only on the durable documents.path column: upsert, save, ingest, move, and the Phase-5 seeder no longer write the virtual_path metadata marker. recorded_virtual_path reads the column first and falls back to the marker only for rows written before migration 189 backfills it, so a re-sync that drops the marker can no longer fork a document. Tests assert on the path column instead of the marker, add coverage that a reindex preserves a row's metadata (no marker is resurrected), and bind converge's per-document session in the artifacts adoption test.
… suite These tests exercise the upload pipeline (ETL, dedup, credits, limits, search) on the Postgres-inline indexer. They inherited the process flag, so a dev .env with KNOWLEDGE_STORE_ENABLED=TRUE born-flipped the test workspace and routed uploads through the git store, whose async convergence no in-process broker runs — hanging the suite locally while CI (flag off) stayed green. Pin the flag off per test so the suite is hermetic; git-native indexing is covered by the knowledge_store and artifacts suites.
…orking The two live writers decided a document's path two different ways. A sync ingest re-attached an unmarked row to the file git already holds for it — the crash window between the commit and the path write-back — while an editor save authored a fresh `name (2).md` and stranded the first. That is a fork, and a steady drift source on flipped workspaces. Move that decision into one place both writers call: `_reattach_or_author_path` re-attaches by the row's identity when git holds the canonical name, else authors. Recorded-path reuse stays in each caller (the record-world check). The seeder, artifacts and `_relocation_of` are left as-is: only an incremental writer of an existing row can reach the recordless-but-file-still-in-git fork. Proven by test_a_resave_of_an_unmarked_row_reattaches_instead_of_forking, the editor twin of the existing ingest re-attach test.
migrate_workspace built its desired set from any body-bearing row without looking at status, so a row with a body but no recorded git path was counted missing whatever its state. A still-pending or failed row therefore alarmed the hourly drift check every run and drew a reindex (git->Postgres) that cannot make a file out of a Postgres row -- a perpetual no-op repair. Skip an unrecorded row whose status is pending/processing/failed: the store was never asked to hold it yet, and the live writer records it once it is ready. A row that already records a path keeps its file whatever its status, so nothing in git is dropped.
Integration tests wrote git repos to the real KNOWLEDGE_STORE_ROOT, a shared on-disk path keyed by workspace id. Because the test schema is recreated per session, test workspaces reuse low ids and overwrote dev workspace repos with the same id, silently clobbering local data. A session-autouse fixture redirects the root to a pytest tmp dir for the whole integration session; a guard test asserts it took.
|
@CREDO23 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
…space magnitude The scheduled parity check emitted a status-only metric (ok/drift/error), which alarms but cannot say how far git and Postgres diverged — that lived only in a log line. Wrap the sweep in a parent span and each workspace check in a `knowledge_store.drift.check` span carrying the missing/extra/mismatched path counts, so a drift alert opens a trace that already quantifies the gap instead of sending someone to grep the worker logs.
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
1 participant
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.
Makes the git-native ingest→index path one reliable writer keyed on the durable
documents.pathcolumn, closing the drift sources found in production.Changes
documents.path; stop stamping the legacyvirtual_pathmarker; backfill the column (migration189).(2).KNOWLEDGE_STORE_ENABLED.knowledge_store.drift.checkspan carrying the missing/extra/mismatched magnitude, so an alert opens a trace instead of the worker logs.KNOWLEDGE_STORE_ROOTto a tmp dir (guarded); pin the upload suite to legacy indexing.Test plan
deleted=0, parity cleanHigh-level PR Summary
This PR establishes a single, reliable write path for git-native document ingestion and indexing by making
documents.paththe authoritative source of truth, eliminating git↔Postgres drift that previously occurred from multiple concurrent writers and transaction boundaries. Key changes include: pinning document paths at ingest time rather than waiting for convergence, re-attaching recordless saves to their existing git files instead of forking duplicates, isolating per-document indexing failures to prevent batch rollback, migrating from the legacyvirtual_pathmarker to the durablepathcolumn (with backfill via migration 189), ensuring signup-created workspaces honor theKNOWLEDGE_STORE_ENABLEDflag, and updating the drift reconciler to skip unrecorded pending/failed rows. The changes consolidate what were previously three independent durability domains (git content, Postgres at ingest, Postgres projection) into a single convergent model where git is the source of truth and Postgres is a self-healing projection.⏱️ Estimated Review Time: 1-3 hours
💡 Review Order Suggestion
plans/git-native-kb/10-ingest-index-atomicity.mdsurfsense_backend/alembic/versions/189_backfill_document_path.pysurfsense_backend/app/knowledge_store/paths/store_path.pysurfsense_backend/app/knowledge_store/paths/__init__.pysurfsense_backend/app/knowledge_store/paths/legacy.pysurfsense_backend/app/knowledge_store/paths/resolve.pysurfsense_backend/app/knowledge_store/service.pysurfsense_backend/app/knowledge_store/index/rows.pysurfsense_backend/app/knowledge_store/index/converge.pysurfsense_backend/app/knowledge_store/migrate.pysurfsense_backend/app/users.pysurfsense_backend/app/agents/chat/multi_agent_chat/main_agent/middleware/knowledge_tree/middleware.pysurfsense_backend/app/agents/chat/runtime/mention_resolver.pysurfsense_backend/app/agents/chat/runtime/references/documents/resolver.pysurfsense_backend/tests/integration/conftest.pysurfsense_backend/tests/integration/knowledge_store/conftest.pysurfsense_backend/tests/integration/artifacts/test_git_adoption.pysurfsense_backend/tests/integration/document_upload/conftest.pysurfsense_backend/tests/integration/knowledge_store/test_backfill_document_path.pysurfsense_backend/tests/integration/knowledge_store/index/test_converge.pysurfsense_backend/tests/integration/knowledge_store/index/test_drift_monitor.pysurfsense_backend/tests/integration/knowledge_store/index/test_project.pysurfsense_backend/tests/integration/knowledge_store/test_folders.pysurfsense_backend/tests/integration/knowledge_store/test_migrate_placement.pysurfsense_backend/tests/integration/knowledge_store/test_path_column.pysurfsense_backend/tests/integration/knowledge_store/test_service.pysurfsense_backend/tests/integration/test_workspace_born_git_native.pysurfsense_backend/tests/integration/test_store_isolation.pysurfsense_backend/tests/unit/knowledge_store/test_recorded_virtual_path.pysurfsense_backend/tests/unit/agents/new_chat/test_mention_resolver.pysurfsense_backend/tests/unit/agents/new_chat/test_path_resolver.pysurfsense_backend/tests/unit/middleware/test_knowledge_tree.py