refactor(knowledge): restructure into domain services and adopt core/job primitives#16749
Open
eeee0717 wants to merge 37 commits into
Open
refactor(knowledge): restructure into domain services and adopt core/job primitives#16749eeee0717 wants to merge 37 commits into
eeee0717 wants to merge 37 commits into
Conversation
Pure moves + import path rewrites, zero logic change. The utils/
grab-bag hid the entire ingestion pipeline; stage directories now sit
at the feature root in pipeline order:
- utils/sources/ -> sources/ (input: directory expansion, url fetch, snapshots)
- utils/indexing/ -> indexing/ (chunk/splitter/embed/rerank/materialFields)
- utils/storage/ -> storage/ (raw/ path allocation)
- utils/cleanup/{subtreePurge,vectorCleanup} -> cleanup/
- utils/cleanup/statusCleanup -> ingestion/ (write-side only consumer)
- utils/addConflicts -> ingestion/
- utils/search -> query/ (read-side ranking helpers)
- utils/items -> items.ts (shared predicates/source probing)
External deep imports updated in KnowledgeMigrator/KnowledgeVectorMigrator
(+ test). vi.mock specifiers in task handler tests updated to match.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
Move the base-lifecycle block (~250 lines) out of the facade into base/KnowledgeBaseAdminService: createBase (with rollback), deleteBase, restoreBase (+toRestoreRuntimeInput), listBases, hasAnyBase, and the deleteBase-only cancelAllJobsForBase. Code is moved verbatim; the facade methods become one-line delegates, so all external call sites and tests are untouched. assertBaseCanRunRuntimeOperation becomes a free function in base/guards so the read side (search/concepts) and the write side can share the same base-status guard without going through the facade. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…sorb admission checks git mv KnowledgeWorkflowService.ts -> ingestion/KnowledgeIngestionService.ts and align the class, handler params, and test mocks with the new name. Admission checks sink from the facade into the ingestion service so one flow's validation + orchestration live in one place: - addItems absorbs the failed-base guard (base/guards) - deleteItems absorbs the outermost-root expansion + empty early-return - reindexItems absorbs the guard, root expansion, and the whole assertSubtreesCanReindex source/status precheck Boot-time recovery (recoverDeletingItems / recoverInterruptedItems) moves in as well — it re-enqueues write-side jobs. Facade methods become pure delegates; behavior and call order are unchanged. The barrel now exports only KnowledgeService (the other exports had no consumers). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…eService Move the read side (~700 lines) out of the facade into query/: - KnowledgeQueryService: search (with visibility filtering, over-fetch trim, rerank) + listItemChunks + listRootItems and their private helpers, code moved verbatim - KnowledgeConceptService: the Concept ID-addressed agent tool surface (readConcept/grepConcept/deleteConcepts/refreshConcepts/ getOrganizationTree) together with its types, constants, and the grep scanner pure functions; delete/refresh delegate to the ingestion service's guarded deleteItems/reindexItems - visibility.ts / storeOperation.ts: loadVisibleItems, deriveConceptId, and runStoreOperation become free functions shared by both classes KnowledgeService is now a pure facade (~150 lines): lifecycle hooks, module assembly, and one-line delegates. All public signatures are unchanged, so external call sites are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
Document the pipeline main axis (sources -> readers -> indexing -> vectorstore, driven by tasks/), the directory map, the five jobs and their chaining/recovery semantics, the item status flow, and what the per-base lock does (and does not) protect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…root
storage/, base/, and cleanup/ each wrapped only 1-2 files around a
directory that added no grouping value — naming-conventions.md §4.4
promotes to a subdirectory only when a topic actually owns multiple
files, and storage/ (a lone pathStorage.ts) had no precedent elsewhere
in the codebase for a non-bucket single-file topic directory.
- storage/pathStorage.ts -> pathStorage.ts
- base/KnowledgeBaseAdminService.ts -> KnowledgeBaseAdminService.ts
- base/guards.ts -> baseGuards.ts (renamed: 'guards.ts' alone at the
feature root lost the disambiguating parent directory name)
- cleanup/{subtreePurge,vectorCleanup}.ts -> root
All four are cross-cutting (consumed by ingestion/ and 3+ task
handlers, not owned by a single pipeline stage), matching the existing
items.ts/types.ts root-file precedent. Brings knowledge/ from 11
top-level subdirectories down to 8, closer to the sibling fileProcessing/
(5) and apiGateway/ (4) feature modules. Pure move + import rewrite +
two internal import-depth fixes in the moved files themselves; no
behavior change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
Auto-fixed by eslint/biome across the flattened knowledge/ layout; no logic changes. Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
Top level now reads as one facade plus five domains: - pipeline/ gathers the four stage dirs (sources/readers/indexing/ vectorstore) under one roof; nothing inside enqueues jobs or mutates item status - base/ owns the per-base domain: KnowledgeBaseAdminService, baseGuards, KnowledgeLockManager - subtreePurge moves into ingestion/ (write-side teardown), vectorCleanup into pipeline/vectorstore/ (pure store maintenance) - types/items.ts merges into items.ts, removing the single-file types/ dir - pathStorage/items/types stay at the root as shared cross-domain vocabulary Pure moves + import rewrites, zero behavior change. README directory map and pipeline diagram updated to match. Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…e calls assertBaseCanRunRuntimeOperation now returns the fetched KnowledgeBase so search()/listItemChunks() no longer re-fetch it, and search()'s token check and chunk-access guard are folded into the FTS layer's own tokenizer / knowledgeItemService.getById instead of duplicating them. listItemChunks() also drops its now-pointless Promise.all/await wrapper now that KnowledgeIndexStore's per-material reads are synchronous (better-sqlite3). Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…esolveConcept assertBaseCanRunRuntimeOperation already fetches the KnowledgeBase; addItems() and resolveConcept() immediately re-fetched it by id right after with nothing but a synchronous check in between, so use the guard's return value instead. Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
markUnscheduledKnowledgeItemsFailed's MarkFailedInput.logContextKey was always passed as the literal 'scheduleError' by both call sites, so drop the field and hardcode the key in the two log payloads it built. Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…ss roots assertSubtreesCanReindex queried each root's subtree and classified its source one at a time in a sequential loop. rootItemIds always come from getOutermostSelectedItemIds, which guarantees the roots are mutually non-descendant, so a single batched getSubtreeItems(..., rootItemIds) call covers every root's subtree in one query, and the per-root source classifications can run concurrently via Promise.all instead of sequential awaits. Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…eporter checkFileProcessingResultJobHandler called ctx.reportProgress(...) directly with raw payload literals in 8 places, instead of the typed reportKnowledgeProgress(ctx, ...) helper every other handler already uses. All payloads already matched KnowledgeProgressDetail, so this is a straight call-site swap. Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…tore better-sqlite3 is a synchronous driver; rebuildMaterial, listMaterialUnits, getMaterialByRelativePath, readMaterialContent, search, reclaimSpace, close, and listExistingEmbeddingHashes were still declared async/Promise<T> from the old libsql-backed store, with zero real awaits in their bodies (only deleteMaterials keeps async — it genuinely yields via setImmediate for large batch deletes). Converting them to plain sync calls is behavior-preserving: await on a non-Promise value is a no-op. Also collapse listExistingEmbeddingHashes and assertEmbeddingCoverage's identical batched SELECT ... IN (...) scans into one shared selectExistingEmbeddingHashes helper parameterized over the SqliteExecutor (driver vs. transaction) each call site already had. Test suites updated in lockstep: Vitest's .rejects/.resolves matchers require an actual Promise, so assertions on the now-synchronous methods switch to expect(() => call()).toThrow(...) / plain expect(call()).toEqual(...), and the now-pointless await at every call site is dropped. Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…ore calls
Follow-on to the KnowledgeIndexStore async→sync conversion: every call site
still awaited rebuildMaterial/listExistingEmbeddingHashes/reclaimSpace/close,
which oxlint's await-thenable rule now flags since the return values are no
longer Promises. Drop the await at each site (indexDocumentsJobHandler,
KnowledgeVectorStoreService.closeStoreInstance — now itself sync,
vectorCleanup.reclaimKnowledgeIndexSpace, KnowledgeVectorMigrator), converting
the wrapping function to sync too where it had no other awaits left.
An unawaited call also changes how a mocked rejection surfaces: a
mockRejectedValue Promise that is no longer awaited becomes an unhandled
rejection instead of a synchronous throw, so a surrounding try/catch written
for the now-sync method never fires. Test mocks for these methods switch from
mockResolvedValue/mockRejectedValue to mockReturnValue/mockImplementation(() =>
{ throw ... }) to match.
Signed-off-by: eeee0717
Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
execute() called this.db.prepare(sql) on every call, recompiling the same SQL text repeatedly for hot paths like the batched embedding-hash scan. Cache compiled Statement objects by SQL text; better-sqlite3 transparently recompiles a cached statement if the underlying schema changes, and assertOpen() already guards against use after close. Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
- items.ts: delete ContainerKnowledgeItemType (zero references) - pathStorage.ts: de-export getKnowledgeBaseMetaDir (only used in-file) - types.ts: de-export KnowledgeWorkflowJobType (only used in-file) - KnowledgeReader.ts: delete ReadableKnowledgeItem, a duplicate of items.ts's IndexableKnowledgeItem; loadKnowledgeItemDocuments now takes the latter - indexStore/types.ts: delete VectorMatch (zero consumers) Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
chunkItemDocuments was a same-signature pass-through to chunkKnowledgeDocuments with no added behavior; call chunkKnowledgeDocuments directly. Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
expandDirectoryNode's `if (node.type !== 'folder') return null` guard was unreachable: the preceding branch already returns for the 'file' case, so TS statically knows node.type is 'folder' at this point. Signed-off-by: eeee0717 Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…peline/
pipeline/sources/prepare.ts's prepareKnowledgeItem/prepareDirectoryForRuntime/
createDirectoryChildren/createRuntimeItem created knowledge_item rows and
flipped item status directly — violating the feature README's invariant that
pipeline/ code doesn't enqueue jobs or mutate item status. Its only consumer,
prepareRootJobHandler's scanRootItem (already inside withBaseMutationLock),
threaded that through two no-op pass-through params:
onCreatedItem (always `() => {}`) and runMutation (always an identity
`async (task) => await task()`), a leftover from an earlier abstraction layer.
Move the file to tasks/prepareItem.ts (its only consumer already lives in
tasks/) and drop the two dead params, calling knowledgeItemService directly
(better-sqlite3 is synchronous, so no wrapper was ever needed).
Also reuse purgeKnowledgeSubtreeWithinLock (already used by
deleteSubtreeJobHandler) in deletePreviousLeafExpansion instead of a
hand-rolled delete-vectors/best-effort-delete-files/delete-rows sequence — the
two now do the exact same three steps in the same order, just no longer
duplicated. purgeKnowledgeSubtreeWithinLock short-circuits for an empty
subtree without calling deleteItemsByIds, unlike the old code which always
called it (even with []); this is a no-op-equivalent behavior change (an
empty-id-list delete does nothing either way), reflected in
prepareRootJobHandler.test.ts's updated assertion.
Signed-off-by: eeee0717
Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…duler interface Job handler factories only need scheduleItem/scheduleIndexing/scheduleFileProcessingCheck, but took the full KnowledgeIngestionService type. Tests had to hard-cast their 3-method mocks with `as never` at every call site to paper over the mismatch. Extract the narrow interface and have KnowledgeIngestionService implement it so handlers depend only on what they use. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…pansion failure expandDirectoryOwnerToTree picks a dedup'd pathPrefix from the DB's existing relativePaths, then copies file bytes into raw/<pathPrefix>/... before any knowledge_item row references them. A failure partway through (disk full, permission error, abort) left those bytes orphaned on disk with no DB row and no reserved name, so the next attempt would reselect the same pathPrefix and fail again with "Knowledge file already exists". Wrap the expansion in a try/catch that best-effort removes the whole pathPrefix directory before rethrowing, so a retry starts from a clean namespace. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
… and job recovery semantics Several docs still pointed at the pre-v2-refactor layout: knowledge-service.md referenced a nonexistent src/main/services/knowledge path, a since-renamed KnowledgeWorkflowService class, and legacy colon-style IPC channel names instead of the current knowledge.* DataApi routes. workflow-architecture.md repeated the stale class name and an architecture diagram still labelled FileManager where knowledge-owned files now go through pathStorage.ts directly. operation-guards.md and README.md described indexing/reindex job handlers as recovery:'retry' when they're actually recovery:'abandon' (interrupted items are parked at failed by recoverInterruptedItems on boot, not silently resumed, to avoid re-spending embedding cost) — only delete-subtree is 'retry'. knowledge-technical-design.md's §4.7 still described joining search_text.rowid = search_text_fts.rowid, the implicit-rowid join #16447's A1 fix deliberately replaced with the stable fts_rowid proxy key. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…ctiveKnowledgeJobs Two independent implementations of "cancel in-flight knowledge jobs (+ their linked file-processing job) in a base's queue" had drifted apart: KnowledgeBaseAdminService.cancelAllJobsForBase (base-wide, used by deleteBase) called jobManager.cancel() directly with no timeout handling and no truthy check on the linked fileProcessingJobId; subtreePurge.cancelActiveKnowledgeSubtreeJobs (subtree-scoped, used by delete-items/replace-on-add) used cancelJobOrThrow and did check fileProcessingJobId. Both run outside the base mutation lock (cancel awaits handler settlement, and the handlers themselves take that lock, so cancelling while holding it would deadlock). Introduce cancelActiveKnowledgeJobs(baseId, reason, options) in tasks/utils/cancel.ts covering both the base-wide and subtree-scoped shapes via an options object (rootItemIds + excludeJobId + onCancelTimeout: 'throw' | 'proceed'), and switch all three call sites (deleteBase, deleteSubtreeJobHandler, and KnowledgeIngestionService's replace-on-add path) onto it. Delete the now-dead cancelAllJobsForBase/cancelActiveKnowledgeSubtreeJobs plus their now-unused imports. Also documents KnowledgeItemScheduler's role, missed when that interface was extracted.⚠️ Side-fix (not pure refactor): the base-wide path (deleteBase) now also skips cancelling an empty-string fileProcessingJobId, matching the subtree path's existing truthy check. Schema allows an empty string in that field in principle, though normal write paths never produce one, so this is a narrow-but-real behavior change, not just a rename. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…solveLiveKnowledgeItem/Subtree "Is this item/subtree gone (NOT_FOUND) or being deleted (status==='deleting'), and if so treat the operation as an idempotent no-op" was hand-rolled ~8 times across prepareRootJobHandler, indexDocumentsJobHandler, checkFileProcessingResultJobHandler, and reindexSubtreeJobHandler, with the classification logic (catch NOT_FOUND / check status==='deleting') repeated near-verbatim each time. Extract only that classification into tasks/utils/liveItem.ts (resolveLiveKnowledgeItem for a single item, resolveLiveKnowledgeSubtree for a resolved root+descendants set) and leave each call site's own logging text, progress stage, and control flow in place — operation-guards.md's convention of each operation keeping its own explicit flow is preserved; only the duplicated classification is deduped. checkFileProcessingResultJobHandler.shouldSkipMissingOrDeletingItem keeps its extra baseId ownership check (an explicit throw, not a skip) outside the shared helper, since that's specific to this one call site. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…ItemService.createActive create() hardcoded status: 'idle' on insert; both production call sites (prepareItem.ts, KnowledgeIngestionService.addItems) immediately followed it with updateStatus(id, isContainerKnowledgeItem(item) ? 'preparing' : 'processing') — the same "container -> preparing, leaf -> processing" rule duplicated twice, plus an extra write transaction in between that was never observably idle. Rename create() to createActive() and compute the active status inline from item.type (mirroring the existing item.type === 'directory' check in reindexSubtreeJobHandler, rather than importing the feature-layer isContainerKnowledgeItem into the data layer). Call reconcileContainers after insert to preserve the container-reactivation side effect that updateStatus's non-failed branch used to trigger — e.g. adding a live child under an already completed directory pulls the parent back to preparing/processing. Both call sites drop their now-redundant updateStatus follow-up call. Also fixes updateStatus's deleting-guard branch: it returned early inside the withWriteTx callback, but the method itself still fell through to the normal "Updated knowledge item status" info log — a blocked update looked identical to a successful one in the logs. Pull the blocked flag out of the transaction and log a distinct warn instead.⚠️ Side-fix (not pure refactor): updateStatus's deleting-guard branch now logs a warn instead of a misleading "Updated" info line when a status update is blocked because the item is being deleted. Return value and DB state are unchanged — this only affects log output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…in index-documents README.md documents that the base mutation lock must only wrap the mutation step of index-documents, never the network fetch or the embedding API call — but no test asserted the ordering, only that both eventually happened. withBaseMutationLock's test double is a plain pass-through (vi.fn(async (_baseId, task) => await task())), so mock.invocationCallOrder already lets us assert fetch/embed happen strictly before the lock is acquired; add those assertions to the two tests that already exercise this path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…antics
The mock's withWriteTx was a pure pass-through (fn(this.db)), even when
setupTestDatabase() had attached a real better-sqlite3 connection via
MockMainDbServiceUtils.setDb() — so tests never got the rollback-on-throw
semantics that production's this.db.transaction(fn, { behavior: 'immediate' })
provides. README.md also documented the mock's signature as async
((fn: (tx) => Promise<T>) => Promise<T>), which doesn't match the synchronous
production signature.
Delegate to the real db's .transaction() when one is attached (detected via
typeof db.transaction === 'function', which only real better-sqlite3
instances have — hand-written mock db literals don't), falling back to the
plain pass-through otherwise. Update the README's documented signature to
match the synchronous production one.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…s against a real temp-dir store The 22-test suite mocked all 10 collaborators (KnowledgeIndexStore, BetterSqlite3Driver, BetterSqlite3VectorIndex, schema.ts, indexMeta.ts, knowledgeItemService, pathStorage, fs, @logger, @main/core/lifecycle) plus hand-copied MOCK_SCHEMA_VERSION to mirror schema.ts's real constant — asserting against the mocks' own call counts rather than the real open/rebuild/evict logic. Rewrite against a real temp-dir SQLite store (mkdtempSync + real openBetterSqlite3IndexDriver, matching the pattern already used by BetterSqlite3Driver.test.ts), keeping only the app-DB and lifecycle boundary mocks (knowledgeItemService, pathStorage, @logger, @main/core/lifecycle). Stale/downgrade-version tests now stamp a real version via direct SQL (`UPDATE meta SET schema_version = ?`) rather than ensureIndexMeta, which hardcodes the current constant and can't write an arbitrary version. Failure-injection tests (evicts-a-failed-open, meta verification, schema creation) use targeted vi.spyOn on the real modules instead of blanket vi.mock. The "single-flight" test is retitled to reflect what getIndexStore's own doc comment already states: the method has no `await` before caching, so a second concurrent call can't observe a genuinely in-flight open — there's no async dedup mechanism to test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…ades, drop idle status Wires the KB module onto the core/job capabilities the maintainer merged in response to issue #16738 (KeyedMutex, typed settled events, list() filters), and clears scaffolding that predated them: - delete KnowledgeLockManager in favor of the core KeyedMutex it duplicated - collapse statusCleanup's updateStatus-then-setSubtreeStatus double-write into a single setSubtreeStatus call (its CTE already guards deleting rows) - desync KnowledgeVectorStoreService's getIndexStore/getIndexStoreIfExists now that better-sqlite3 makes them synchronous under the hood - drop the unreachable idle KnowledgeItem status; add a never_indexed error code for the migration-mapping branch that used to fall back to it - swap KNOWLEDGE_ACTIVE_JOB_STATUSES for the shared ACTIVE_JOB_STATUSES - read onSettled's typed event.input directly instead of re-fetching the job - drop the redundant input.parentJobId payload field now that ctx.parentId carries the same value - push parentId/type filters into JobManager.list() instead of filtering results in JS This is the first-two-layers slice of a maintainer-invited follow-up checklist; enqueueTx adoption and the two 5000-job-limit loop-until-drained fixes are intentionally left for a later pass. Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…n cancel beyond the page limit deleteItems previously set items to 'deleting' inside the base lock, then enqueued the delete-subtree job in a separate step with no rollback on failure — a failed enqueue left items stuck in 'deleting' forever with no job to clean them up. Split KnowledgeItemService.setSubtreeStatus into a Tx variant and run the status update + JobManager.enqueueTx together in one write transaction, so either both commit or both roll back. cancelActiveKnowledgeJobs capped its job listing at KNOWLEDGE_ACTIVE_JOB_LIMIT (5000), silently leaving any excess active jobs uncancelled. Reinterpret the constant as a page size and loop until a page comes back short, re-querying from the top each round since cancelling shrinks the active-job set between rounds and list()'s createdAt-only ordering has no stable offset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…one shared schema create()/update() each hand-rolled their own subset of the cross-field rules already expressed in KnowledgeBaseSchema's superRefine, so the embedding/dimensions pairing, no-model-bm25, and chunk-overlap checks were defined 2-3 times with drifting wording, and no boundary validated the delimiter/separator pairing at all. Extract the refine callback into refineKnowledgeBaseInvariants, reuse it for a new KnowledgeBaseWriteSchema covering the pre-write candidate row, and have create()/ update() validate their about-to-be-written row against it via safeParse + toDataApiError instead of their own DataApiErrorFactory.validation calls. Add the missing delimiter/separator check, and align update()'s two status-gated rules (embedding pairing and no-model-bm25) to both fire only when completed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…read processedRelativePath through job payload Eliminates a query->pipeline reverse import (toMaterialRelativePath now lives in the shared items.ts) and removes the fp-check job's absolute-path rederivation in favor of passing the already-known processedRelativePath through the job payload, deleting the now-dead toKnowledgeRelativePath/ isPathInsideBase helpers. Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…ose crash-safety gap Directory expansion durably copied file bytes into raw/<pathPrefix>/ before pinning the container's relativePath or committing any child row. A crash/kill in that window left orphan bytes that no retry could reclaim: the retry's prefix selection only counts DB rows (orphan bytes contribute nothing), re-picks the same prefix, and assertTargetAvailable then hits "Knowledge file already exists" in a permanent failure loop. The retry purge walks getSubtreeItems, which excludes the container row, so the container's own raw/<prefix> shell was never swept. Design P (pin-first + explicit container-prefix reclaim on retry): - directory.ts: split the pure prefix chooser (chooseDirectoryPathPrefix) out of expandDirectoryOwnerToTree; the expander now takes pathPrefix and returns the node array. Drop the stale in-function try/catch cleanup. - prepareItem.ts: pin updateDirectoryRelativePath BEFORE any byte is copied. - prepareRootJobHandler.ts: after the descendant purge, removeDir the container's own raw/<relativePath> shell (idempotent, same runExclusive lock). Invariant: the pin strictly precedes the first copy, so any window that holds orphan bytes has the container row already pinned to those bytes' prefix -> the retry removeDir deletes a superset of the orphans. happy-path behavior is unchanged; empty/failed directories now reserve their top-level name (a failure-path change we accept as more correct). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…sses the subtree cancelActiveKnowledgeJobs drained the active-job queue by re-listing a full page until the raw page length dropped below KNOWLEDGE_ACTIVE_JOB_LIMIT. For a scoped (subtree) cancel, the subtree filter runs only in memory, so a full page whose every job misses the scope cancelled nothing, never shrank the active set, and re-queried the same page forever — hanging delete-subtree/replace-on-add and busy-spinning a core. Break when the page comes back short OR when a full page yielded nothing cancellable this round, so a scoped cancel with no in-scope jobs stops instead of looping. Base-wide drain behavior is preserved (a full page of matching jobs still loops until drained). Adds a regression test that returns the same all-miss full page on every list() call — it hangs on the old code and asserts a single list() with no cancels on the new. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…e refactor This branch dropped the idle item status and the KnowledgeLockManager class (the per-base lock is now a core KeyedMutex instance created by the KnowledgeService facade) and moved several helpers/methods, but prose and comments still referenced the old shapes. - Remove idle from status lists (knowledge-service/workflow-architecture/ operation-guards) and code comments; the migrator maps no-uniqueId items to failed (never_indexed), so its README now says so instead of idle. - Replace KnowledgeLockManager references with the concrete per-base mutation lock / KeyedMutex.runExclusive wording, drop the deleted withBaseMutationLock method reference, and correct the base/-owns-the-lock factual error (the facade creates it and shares it). - Fix stale symbol references: toKnowledgeRelativePath -> toMaterialRelativePath, KnowledgeService.recoverInterruptedItems -> KnowledgeIngestionService, and move the material-path derivation note from pipeline/indexing/ to items.ts in the feature README directory map. - Correct the feature README query/ role: it is not purely a read side — the Concept ID tool surface (KnowledgeConceptService) also exposes kb_manage delete/refresh writes that delegate to ingestion/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
…pi error helpers query/visibility.ts imported isDataApiNotFoundError from tasks/utils/settled.ts, a query -> tasks reverse dependency that broke the read side's downward-only layering. The predicate is a fully generic DataApi check (isDataApiError(e) && e.code === NOT_FOUND) with no task-specific logic, so it belongs next to its siblings isDataApiError / isRetryableErrorCode in @shared/data/api/apiErrors.ts rather than in a knowledge task util. Move it there (verbatim, still a boolean predicate — no speculative type-guard narrowing) and re-point all five consumers (settled, liveItem, prepareRoot and indexDocuments handlers, query/visibility) at @shared/data/api. query/ now depends only downward on @shared, closing the leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
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.
What this PR does
Before this PR:
KnowledgeServicewas a ~1,186-line god object; pipeline stages, the vector store, and readers lived under an ad-hocutils/plus scattered top-level dirs.core(KeyedMutex, typed settled events,enqueueTx,listfilters), and dual-encodedparentJobId.raw/<prefix>/before pinning the container'srelativePath, so a crash mid-expansion left orphan bytes that a retry could never reclaim (permanent "file already exists" failure loop);KnowledgeBaserow invariants were validated in three divergent places; the deadidleitem status lingered; index-store facades still carried libsql-eraasync.After this PR:
KnowledgeServicesplit intobase/ingestion/query/conceptservices (facade down to ~154 lines);utils/+vectorstore/+readers/consolidated underpipeline/; the feature is grouped into domain directories with a README.KeyedMutex; typed settled events (drop thegetJobrefetch and theparentJobIddouble-encoding); pushparentId/type[]filters intolist()and use the sharedACTIVE_JOB_STATUSES; rundeleteItemsthroughenqueueTx(closes a delete-marking/enqueue atomicity gap); change cancellation to loop-until-drained (drops the silent 5,000-job cap).relativePathbefore any byte copy and reclaims the container prefix on retry, closing the crash-safety gap;KnowledgeBaseinvariants converge into one shared ZodsuperRefineplus a new chunk-separator rule.idlestatus end-to-end; unify addressing (toMaterialRelativePathhoisted intoitems.ts,processedRelativePaththreaded through the job payload); desync the index-store facades now that better-sqlite3 is synchronous; drop dead code.Related to #16738 — this wires up the KB-side consumer adoption (the maintainer's checklist items 1–7) of the core/job capabilities that were accepted and merged. Item 4 (
finished(jobId)/dependsOn) remains deferred upstream, so the issue stays open and this PR does not close it.Why we need it and why it was done in this way
This branch has two arcs. About 8,700 of the changed lines are pure file relocations (the
utils/+vectorstore/+readers/→pipeline/moves and the god-object split) with no semantic change; the genuinely edited logic is roughly 1,200 lines. They are kept in one PR because the structural moves are what let the convergence edits land in their final home cleanly.The following tradeoffs were made:
docs/references/knowledge/workflow-architecture.md) and the per-phase job policies (timeout/retry) rather than collapsing the three per-item jobs into one reconcile job.The following alternatives were considered:
prepare-root/index-documents/check-file-processing-result) into one phase-derived reconcile job (P10) — rejected: it fights the documented dumb-handler invariant for near-zero net line savings, and its one real payoff is gated on a core capability that is not landing.finished(jobId)to await a sibling job — rejected: the maintainer deferred it in [Feature]: 补齐 core/job(JobManager)的类型化 settled 事件、级联取消与跨 job 等待能力 #16738 (the correct shape is DB-encodeddependsOn, needs its own RFC), and the polling chain is an intentional durable-timer pattern, not debt.Links to places where the discussion took place: #16738 (maintainer ruling + KB-side checklist).
Breaking changes
None. Job types are internal with no renderer/telemetry consumer, and there is no external contract or default-value change. The
migrations/diff is a regenerated dev migration (idle-status drop + the P9 schema change) and will be squashed into the single clean initial migration before release.Special notes for your reviewer
utils/+vectorstore/+readers/→pipeline/moves and the service extraction are semantics-preserving; (2) the logic arc — the ~1,200 lines of real edits (directory crash-safety,KnowledgeBaseinvariant convergence, addressing unification, core/job wiring, guard unification).migrations/are the regenerated dev migration, not meaningful review surface.pnpm test/pnpm lint/pnpm format/pnpm build:checkall pass locally.Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
mainfor active development,v1for v1 maintenance fixes/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note