[Feat] Community Local: workspace & document API, hybrid search index - #1742
Merged
Conversation
Layout and markers now match surfsense_backend, down to pytestmark at the top of each module rather than a decorator per test. Only test_config is a unit test by cloud's own definition of the marker: everything else runs against a real SQLite file built by the migrations. Nothing here is mocked, so integration means what it says.
Relationships name their target as a string, so a slice the app never imported is a name SQLAlchemy cannot resolve: Workspace.chat_threads points at a class the API had no reason to import, and every query against workspaces or documents would have returned 500. The suite could not see it. conftest imports every slice for the drift test, which left the mappers configured in tests and unconfigured in the process a user runs, so the guard boots the app in a subprocess.
The shell needs all five: a switcher lists and creates, a deep link reads one by id, and a workspace named by typo has to be fixable or removable. Nothing else in the app deletes a workspace. passive_deletes hands the cascade to SQLite, which the migration already declares. Without it SQLAlchemy loads every child row to delete it one at a time: 502 statements for a workspace holding 500 chunks, against 1. Names are stripped before the length check, so a name of spaces fails rather than becoming a row that renders blank. Handlers are sync def, which FastAPI runs in a threadpool, because the session is sync and would otherwise block the event loop. The client fixture wires a session factory itself: ASGITransport does not run lifespan.
NOTE was in the enum with no way to create one. A note is a document the user writes directly, so it needs no upload and no worker, and it starts pending because ready means indexed, not stored. Only a note's body is editable; anything else was extracted from bytes and the edit would vanish on the next ingest. Editing one returns it to pending, since the indexed copy is now stale. error_message joins the initial migration rather than a second one, as nothing has shipped. The worker promises "failed + message" and the documents view promises to show it, but cloud kept that reason inside a JSONB status blob and flattening status to a TEXT enum dropped it. The list carries no content: it is polled while ingest runs, and a body per row would ride along every time. It takes document_type and status filters so Studio output can be told from uploads, and limit/offset so a large workspace is not re-sent on every poll.
The plans named four routes for these two resources. Everything else the UI needs was never written down: rename, delete, reading one document, and the retry that a failed ingest has no other way back from, since re-uploading the same bytes is rejected by the dedup index. 00c-data-model.md now carries the whole table in place of the line that gestured at it, plus the list and edit semantics. Phase 2 picks up the three that need a file on disk, and is told to extend both deletes: chunks_fts and chunk_vectors are virtual tables with no foreign keys, so no cascade reaches them and the index would keep answering for documents the user deleted.
Nothing in the plans said what had been built, so the only way to answer it was to read the tree. The index is where anyone picking up a workstream already looks. Phase 1 carries a marker on two of three cells and a line naming what it still owes, because "started" over a phase that large says almost nothing on its own.
chunks_fts and chunk_vectors, with triggers on chunks that carry inserts, updates and deletes into both. Neither takes a foreign key, so without them a deleted document stays searchable forever, and the triggers fire on cascade too, which is how every real delete arrives: the user removes a document or a workspace, never a chunk. The delete routes need no cleanup code at all. fts5 is compiled into SQLite; vec0 is not, so shared/db.py loads the extension on each connection. It registers per connection rather than in the file, and the worker's connections need it as much as the API's. A vec0 table is fixed at the width it is created with, and vectors from another model are not the wrong shape but unrelated numbers. Startup compares the declared width against the setting and refuses a database that no longer matches, rather than ranking one against the other. Both spread over shadow tables no model declares, so the drift test now compares everything except those.
Huey on its own SQLite file: the consumer polls constantly, and pointing it at surfsense.db would leave it holding the write lock against the requests it is meant to serve. The task is declared beside the documents it ingests, because Huey binds a task to its queue at decoration and the API is what enqueues it. The body belongs to the worker, and nothing drains the queue yet. conftest sets a temporary data directory before its first import, since the queue opens its file as the module loads and no test may write to the developer's own ~/.surfsense. That import is also where a fresh machine failed: nothing had created the directory, so the app died before a route existed. The boot test now runs against a path that does not exist.
Streamed to disk a megabyte at a time and hashed on the way past, so a 400 MB PDF is never held whole. Cloud reads each file into memory, writes a temp copy, then reads that back to store it: three passes and two copies resident. dedup_key is the hash of the bytes rather than of the filename. Keyed on the name, the same report saved twice ingests twice, while two unrelated files both called report.pdf collide and the second is refused. Only a plain extension is taken from what the client sent; the path is built from row ids, so nothing a user types reaches the filesystem. The size is counted as it streams, since a length the request declares is the client's word for it. Enqueued after the commit, or the worker looks for a row this request has not written. A batch is split rather than rejected, so a dropped folder holding one known file keeps the rest. Retry exists because failed is otherwise terminal: the same bytes uploaded again are a duplicate. Deletes take the document directory, after the commit that a rollback would undo. The index needs no cleanup: its triggers already follow the cascade.
API phase 2 is done, so its plan says what was built rather than what to build, and the two worker phases lose the items it took with it: the queue exists, and ingest no longer writes the keyword index because a trigger does. Packaging gains the trap this uncovered. PyInstaller bundles what an import statement names, and vec0.so is opened by path from C, so it is dropped and the frozen app cannot open its own database. Same for the alembic revisions and Docling's models. The spike now has to open a real database, which is where that costs an afternoon instead of a release.
|
@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: Team 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 |
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.
Continues #1741. Everything here is under
surfsense_local/; nothing outside it is touched.What this adds
The whole API surface for workspaces and documents. Full CRUD on both, plus notes — a document the user writes in the app with no file behind it, which the enum allowed but nothing could create. The list takes
document_typeandstatusfilters so Studio output can be told from uploads, andlimit/offsetso a large workspace is not re-sent on every poll. It carries nocontent: it is polled while ingest runs, and a body per row would ride along each time.Upload, retry, and serving the original. Streamed to disk a megabyte at a time and hashed on the way past, so a 400 MB PDF is never held whole.
dedup_keyis the hash of the bytes rather than of the filename as in cloud — keyed on the name, the same report saved twice ingests twice, while two unrelated files both calledreport.pdfcollide and the second is refused. Only a plain extension is taken from what the client sent; the path is built from row ids. Jobs are enqueued after the commit, or the worker looks for a row the request has not written yet.The hybrid search index.
chunks_ftsandchunk_vectors, with triggers onchunksthat carry inserts, updates and deletes into both. Neither takes a foreign key, so without them a deleted document stays searchable; the triggers fire on cascade too, which is how every real delete arrives. The delete routes need no index cleanup at all. Avec0table is fixed at the width it was created with, so startup refuses a database whose width no longer matches the configured model rather than ranking unrelated vectors against each other.The queue. Huey on its own SQLite file, so the consumer's polling does not hold the write lock against the database serving requests. Nothing drains it yet — that is the worker's phase — which is why an uploaded document sits at
pending.Two bugs found on the way
A relationship naming its target as a string left
Workspace.chat_threadsunresolved in the running app, because the API had no reason to import that slice. Every query against workspaces or documents would have returned 500. The suite could not see it:conftestimports every slice for the drift test, so the mappers were configured in tests and not in the process a user runs. There is now a boot test in a subprocess.SqliteHueyopens its file as the module is imported, so on a machine with no~/.surfsensethe app died at import before any route existed. Again invisible to the suite, whose fixture had already created the directory. The boot test now runs against a path that does not exist.Notes for review
tests/unit/<slice>andtests/integration/<slice>to matchsurfsense_backend. Nothing is mocked: integration means a real SQLite file built by the migrations.sqlite_vec/vec0.sois opened by path from C, so PyInstaller will drop it and the frozen app will not open its own database. Recorded inapi/05-packaging.md, and the phase 0 spike now has to open a real database.High-level PR Summary
This PR implements the complete workspace and document API surface for the community local edition, including full CRUD operations, file upload with streaming and deduplication, and a hybrid search index (FTS5 + sqlite-vec). The implementation adds database triggers to keep search indexes synchronized, introduces a Huey-based task queue for background processing, and includes 60 tests covering both unit and integration scenarios. Two critical bugs were discovered during development: unresolved SQLAlchemy relationships due to missing model imports, and premature file opening in
SqliteHueythat would crash on machines without~/.surfsense. The changes are entirely isolated tosurfsense_local/with no modifications to existing cloud infrastructure.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
plans/community-local/00-umbrella-plan.mdplans/community-local/00c-data-model.mdplans/community-local/api/00-spike.mdplans/community-local/api/01-skeleton.mdplans/community-local/api/02-upload.mdplans/community-local/api/05-packaging.mdplans/community-local/frontend/01-shell.mdplans/community-local/frontend/02-documents.mdplans/community-local/worker/01-boot.mdplans/community-local/worker/02-ingest.mdsurfsense_local/backend/pyproject.tomlsurfsense_local/backend/uv.locksurfsense_local/backend/shared/config.pysurfsense_local/backend/shared/queue.pysurfsense_local/backend/shared/db.pysurfsense_local/backend/shared/migrations.pysurfsense_local/backend/alembic/versions/0001_initial_schema.pysurfsense_local/backend/modules/workspaces/models.pysurfsense_local/backend/modules/workspaces/schemas.pysurfsense_local/backend/modules/workspaces/dependencies.pysurfsense_local/backend/modules/workspaces/router.pysurfsense_local/backend/modules/documents/models.pysurfsense_local/backend/modules/documents/schemas.pysurfsense_local/backend/modules/documents/dependencies.pysurfsense_local/backend/modules/documents/storage.pysurfsense_local/backend/modules/documents/tasks.pysurfsense_local/backend/modules/documents/router.pysurfsense_local/backend/modules/chat/models.pysurfsense_local/backend/modules/artifacts/models.pysurfsense_local/backend/api/main.pysurfsense_local/backend/tests/conftest.pysurfsense_local/backend/tests/unit/shared/test_config.pysurfsense_local/backend/tests/integration/test_migrations.pysurfsense_local/backend/tests/integration/test_app_boot.pysurfsense_local/backend/tests/integration/workspaces/test_routes.pysurfsense_local/backend/tests/integration/documents/test_routes.pysurfsense_local/backend/tests/integration/documents/test_upload.pysurfsense_local/backend/tests/integration/documents/test_constraints.pysurfsense_local/backend/tests/integration/chunks/test_constraints.pysurfsense_local/backend/tests/integration/chunks/test_search_index.pysurfsense_local/backend/tests/integration/chat/test_constraints.pysurfsense_local/backend/tests/integration/artifacts/test_constraints.pysurfsense_local/backend/tests/integration/health/test_health.py