feat(jobs): filter job listing by namespace and add job deletion - #231
feat(jobs): filter job listing by namespace and add job deletion#231SusannaShu wants to merge 2 commits into
Conversation
Adds two capabilities the multi-tenant proxy in keyboard-backend needs to
own its own view of a user's uploads.
Namespace filtering
- `GET /v1/jobs` and `GET /v2/jobs` accept `namespace`, matched against
`job_metadata ->> 'namespace'`.
- The filter is normalized the same way job creation normalizes it, so a
blank or whitespace value means the `default` namespace instead of
silently matching nothing. Omitting the parameter still lists every
namespace.
- Jobs written before the namespace was recorded have no key at all;
those count as `default` so the filter does not hide them.
- `JobReadModel.list_jobs_for_user` now accepts and forwards `namespace`.
Without this the route raised TypeError on every list call.
Job deletion
- `DELETE /v1/jobs/{job_id}` and `DELETE /v2/jobs/{job_id}` soft-delete a
job via a new `jobs.deleted_at` column. The row is retained so in-flight
workers, billing records, and audit history are unaffected; the job just
stops appearing in the read APIs.
- The linked document is archived by default (`archive_document=false` to
opt out), but only when no other live job still targets it, so
re-parsing a document does not lose its content.
- Deleting twice returns 404 the second time, matching the behavior of an
unknown job. Soft-deleted jobs are also invisible to GET and to
confirm-upload.
Adds partial indexes for the two new query shapes: a user's active jobs by
creation time, and active jobs by namespace.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The namespace-filter tests created two jobs over HTTP, which tripped the job-creation rate limit in CI and returned 429 on the second create. They now insert job rows directly via ContractDatabase, so each test makes a single request and the namespace fixture is explicit. Adds coverage for a job whose job_metadata has no namespace key at all, which the default filter must still match. Also drops the unused _DEFAULT_RETRIEVAL_NAMESPACE alias flagged by CodeQL; nothing referenced the private name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for explaining the keyboard-backend use case. We understand the underlying requirement: the backend needs to list the files belonging to one tenant and remove a file from that tenant’s document collection without maintaining its own list of Knowhere job IDs. However, We do not think Job is the correct resource for this behavior. In Knowhere, a job represents a document-processing attempt. It is part of the permanent operational ledger and preserves processing state, failures, billing, timing, provenance, and audit history. It should remain available even after the resulting document is archived or deleted. Soft-deleting a job still hides part of that history and conflates two different lifecycles. namespace is meaningful for the persisted document corpus, not for managing processing attempts. A job may record the namespace as immutable provenance, but clients should not use GET /jobs?namespace=... as their document-listing interface. We believe the requested client workflow should use the document resources instead:
For uploads that have not produced a document yet, keyboard-backend should retain the returned job ID while processing is in progress. If it needs durable reconciliation of pending submissions, we should Therefore, We agree with the business requirement, but We do not think namespace-filtered job listing or job deletion is the right interface. Could you revise the change around the document model and |
|
Agree, please reject this PR, thanks. |
Why
A multi-tenant proxy (our keyboard-backend) fans every end user's uploads into a single Knowhere account, separated only by
namespace. Two gaps made that proxy unable to own its own view of a user's files:GET /v1/jobshad no way to scope results to one namespace, so the proxy had to keep its own job-id list and issue oneGET /v1/jobs/{id}per file.DELETE /v1/jobs/{id}returned500 Method not allowed, so callers had no delete path and had to hide jobs client-side, which meant they reappeared on the next sync.What changed
Namespace filtering
GET /v1/jobs,GET /v1/jobs/page, and the v2 equivalents accept anamespacequery parameter, matched againstjob_metadata ->> 'namespace'.Three behaviors worth calling out:
normalize_retrieval_namespace, so an unnormalized filter value would silently match zero rows.?namespace=and?namespace=%20%20now mean thedefaultnamespace.default. Rows whosejob_metadatahas nonamespacekey are matched by the default filter rather than being invisible to every query.This also fixes a latent break in the same code path:
JobReadModel.list_jobs_for_userdid not acceptnamespace, soread_serviceforwarding it raisedTypeErroron every list call. That method now accepts and forwards it.Job deletion
DELETE /v1/jobs/{job_id}andDELETE /v2/jobs/{job_id}.Deletion is soft, via a new
jobs.deleted_atcolumn:GET /jobs,GET /jobs/{id}, andconfirm-upload.check_job_permissionraisesNotFoundafter the ownership check, so deletion never leaks whether a job exists.The linked document is archived by default so the content leaves retrieval, with
?archive_document=falseto opt out. Archiving is skipped when another live job still targets the same document, so re-parsing a document and then deleting the older job does not wipe the current content.Response:
{ "job_id": "job_abc", "deleted": true, "document_id": "doc_xyz", "document_archived": true }Indexes
Two partial indexes for the new query shapes, both
WHERE deleted_at IS NULL:idx_job_user_active_created_aton(user_id, created_at)— the default listing.idx_job_user_namespaceon(user_id, (job_metadata ->> 'namespace'))— namespace-scoped listing.Migration
fce1d2e3f4a5, on top offbe1c2d3e4f5. Adds the nullabledeleted_atcolumn and the two indexes. No backfill needed — existing rows are active by definition.The repo already had two alembic heads before this change (
f0d85d209e68andfbe1c2d3e4f5); this branch does not change that count, and tests/deploys useupgrade heads. Worth a separate cleanup, out of scope here.Tests
tests/contract/test_job_delete_contract.py— delete hides the job from GET and list, other jobs survive, double delete is 404, unknown job is 404,archive_document=falseis honored.tests/contract/test_job_read_contract.py— namespace filter isolates tenants, omitting it lists everything, blank meansdefault.tests/migrations/test_schema_contract.py—deleted_atand both indexes exist afterupgrade heads.Verification status
make lintandmake typecheckboth pass clean. Route registration, the generated namespace SQL, and the alembic revision graph were verified directly.The contract tests were not executed locally — this machine has no
libpqor Postgres server binaries, sopytest_postgresqlcannot start, and that blocks collection of the whole suite (pre-existing, not introduced here). They need a CI run to confirm.The delete service's control flow was exercised against stubbed repository and document services, covering all four paths: archive, skip-archive-when-shared, already-deleted →
NotFound, and wrong-owner →PermissionDenied.Compatibility
Additive. New optional query parameter, new endpoint, new nullable column. No existing response shape or default behavior changes.
🤖 Generated with Claude Code