Repository navigation
perf(search): reuse unchanged index source entries - #1301
Conversation
|
No confident findings in the specified PR diff. The PR description satisfies the testing requirement: it records a passing real-CLI E2E test covering initial indexing, unchanged-source reuse, and an upstream document edit. One representative run is sufficient. Review was read-only; I did not run, build, or install PR code. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, this is a solid change. Project scope does what #1239 asks. One acceptance item is still open in user scope.
We built this head and its base 97f1e7b4 and ran teamai pull with a read probe on the index build. We ran add, edit, rename, delete and vote steps. At every step the index equals a full rebuild. An index written by either build is read correctly by the other.
[P1 blocking] In user scope, an unchanged pull still reads every learning. The triage on #1239 put the criterion as "on an unchanged pull, the index build reads and tokenizes no source file". It also named the user-scope mirror as the case that defeats a timestamp fingerprint.
- Scenario: user scope, 1,000 learnings, no change. Three pulls in a row each make 2,000
readFilecalls from the index build on~/.teamai/learnings/*.md. Project scope makes 0. - Cause:
src/utils/learnings-mirror.ts:111and:115copy every mirrored file withoverwrite: trueon each pull. That resets their timestamps, so the fingerprint misses (search-index.ts:521). The file is read, the hash matches, and only tokenizing is skipped. - At 5,000 learnings an unchanged user-scope pull takes 336 ms, against 92 ms in project scope. The base takes 1476 ms.
- Two ways out. One is to make the mirror skip copies that are already identical and keep their timestamps. A scratch version of that took the reads from 2,000 to 0 and kept the index equal to a full rebuild across an edit and a delete. The other is to change
Fixes #1239toRefs #1239and state the user-scope limit. Either way, the user-scope e2e should assert a read count, not only the reuse counts in the log.
Not blocking:
[P2 non-blocking]The index file grows. With 5,000 learnings it goes from 2.90 to 5.18 MB in project scope, and from 2.19 to 7.41 MB in user scope. Every pull parses it. In user scope the growth comes from storing the shadowed copies from the inherited root as full entries. Please state the size cost in the body, and consider leaving unselected copies out of the cache.[P2 non-blocking]FILE_DERIVATIONchanges only with the schema, Node or ICU. A future tokenizer change keeps stale tokens unless someone bumps1:by hand, and the triage noted derivation has changed before without a bump. Tying it to a version the build already has would close that.[P2 non-blocking]Two changes #1239 did not ask for. TheSearch index build took … consider incremental updateswarning is removed. Carried-over (unreadable) entries now get current votes, and no test covers that.
- Cache parsing and tokenization per source path, entry id and category, with inode, nanosecond mtime/ctime and size fingerprints plus a content hash for metadata-only changes. - Invalidate cached derivations after schema, parser revision, Node or ICU changes; recompute source precedence, selection, votes, confidence and document frequencies on every build. - Reference selected index entries from the file cache instead of storing their tokens twice, preserve atomic publication and log reused, rebuilt and dropped counts. - Add incremental/full equivalence, deletion, restored-mtime, selection and vote regressions, bilingual guidance and a real-CLI pull/update E2E; pass 159 focused tests, build, typecheck and lint.
- 镜像内容相同时跳过覆盖,保留共享和命名空间 learning 的时间戳,避免每次用户范围 pull 触发索引重新读取。 - 添加时间戳回归和真实 CLI 文件读取探针,连续三次未变更 pull 对 learning 的索引读取为零;修改 learning 后验证探针能检测读取并更新索引。 - 修复前时间戳用例失败;修复后 13 个相关单测、真实 CLI 用例、构建、类型检查和 lint 通过。
a981597 to
11868a8
Compare
|
Addressed the review follow-up at
The PR description contains the updated verification record. Ready for another review. |
The PR description satisfies the testing requirement: it records a passing real-CLI E2E run and follow-up verification of user-scope cache reuse. One representative run is sufficient. Review was read-only; I did not run, build, or install PR code. |
- 仅在文件内容与权限均相同时保留镜像元数据,确保团队将脚本改为可执行后本地镜像及时更新权限。 - 新增非 Markdown 附件的 chmod 回归,修复前复现权限未传播,修复后通过。 - 14 个相关单测、真实 CLI 零索引读取与更新探针、构建、类型检查和 lint 通过。
|
Addressed the review follow-up at
The PR description contains the updated verification record. Ready for another review. |
|
No new confident findings in the specified PR diff.
|
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, approving at 8d64882. The blocking finding is fixed: the mirror no longer rewrites identical learnings, so their fingerprints hold. With 1,000 user-scope learnings, an unchanged pull now makes 0 reads of learnings from the index build, against 3,000 per pull on the base. Stale entries still rebuild with the built CLI in user scope: a same-size edit arriving through git with its old mtime restored, and an in-place same-size write keeping the inode, are both picked up, because ctime changes. recall finds the new tokens and no longer finds the old one. The PR's unit and e2e tests pass, also with a symlinked TMPDIR, and 4 unit tests and the e2e fail on the merge-base. Build, tsc and lint pass.
- [P2 non-blocking] A change to how entries are derived serves stale entries unless someone bumps
FILE_DERIVATIONby hand. With a build patched to changetitleFromFilenameand no bump, the next pull logged3 reused, 0 rebuilt, the index kept the old title, andrecallmissed the file. The base rebuilt it. That makes the note intokenizer.tsand the body's "derivation … changes invalidate reuse" misleading. Including the CLI version (getCurrentVersion()) inFILE_DERIVATIONwould reparse once per release and fail safe. - [P2 non-blocking] Carried-over votes have no test.
- [P3 nit] The unchanged check trusts stat alone, so a same-size rewrite within one timestamp tick, or on NFS, could be missed. The user-scope index is about 2.4x larger (shadowed copies stored in full), which costs about 7 ms of parsing per
recallat 5,000 learnings. The "Search index build took" warning is removed without a mention in the body.
The CI review reports no new findings. We agree that nothing blocks, but it ran no code and did not see the derivation case above. All CI jobs pass.
…1306 Conflicts, resolved so both features hold: - step-state.ts: RunningSchema takes Tencent#1303's expiresAt and limitsFrom; DeliveringSchema and the shared variant list stay. CancelledSchema keeps settledFields, which now carry Tencent#1303's unenforced. cancel() keeps a delivering step's settlement and uses Tencent#1303's stoppedLease for a leased one. stopWhere, which Tencent#1303 moved here, cancels a delivering step as before. - engine.ts: this branch's stopWhere is dropped for Tencent#1303's. The drain, the gate and the report vetting take the run's secrets and reconcile with a time, as updateRun, queueRunRecord and reconcileRun now require. leaseNext resolves the env before the drain, so its writes mask the same secrets. parseReport reads unenforced and still names unread changes fields. A report marks its copy used and may start a delivery. - types.ts, snapshot.ts: copies hold from and used; deliveries, creating, releasedTokens, stoppedAttempts, limitsFrom, unenforced and expiresAt each keep a field rule. - conformance.ts, conformance-interpreter.ts and both runners: the runner moved into Tencent#1303's interpreter. Scenarios send base instead of workspace: true, since every profile can take workspaces, and the interpreter fills a report's changes from its saved lease. - parse.ts, records.ts, run-controls.ts, workspace.ts, the handoff and engine tests and docs/dev/workflow-engine.md keep both sides. - Tests call startRun, cancelRun, retryStep and updateRun with secrets, and pass workspace modes in can.
Every pull currently rereads and tokenizes every knowledge file, including the unchanged-revision path. This caches per-file derivations while recomputing selection, source precedence, votes, confidence and document frequencies on every build. Deletions and inactive namespaces disappear from both the index and its cache.
Fingerprints include device/inode, size and nanosecond mtime/ctime. Metadata-only changes compare content hashes before reusing tokens. Schema, derivation, Node or ICU changes invalidate reuse. Selected entries are referenced instead of storing their tokens twice. Publication remains atomic, with reused/rebuilt/dropped counts in the debug log.
Verification
npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/incremental-index.test.tspassed. Three real pulls in an isolated HOME with a local Git remote built the index, reused unchanged sources (0 rebuilt), then indexed an upstream doc edit.git config --get hook.teamai-post-checkout.commandis missing in this environment.Single local-disk samples with ~2 KB Markdown files while the unit suite was running:
NFS was unavailable. Enumeration/stat, index loading, frequency aggregation and atomic serialization remain O(N); tests assert that unchanged sources are neither read nor retokenized. Coverage includes same-size edits with restored mtime, deletion, category/selection changes, votes, and equivalence with a full build.
Fixes #1239
Follow-up verification (2026-10-25)
Fixed user-scope cache misses caused by the learning mirror overwriting identical files. Root and namespaced mirror copies now retain their metadata only when both bytes and permissions are unchanged. Upstream executable-bit changes still propagate to namespace attachments.
Added a timestamp regression (red before the fix) and a real built-CLI read probe: three unchanged user-scope pulls produce zero learning reads from
entryFromMdFile. Editing the learning then triggers reads, proving the probe is active, and updates the index.Learning-mirror and incremental-index unit files: 14 passed. Built-CLI incremental-index E2E: 1 passed. Build, type checking and lint passed.
Cost note: the incremental cache stores reusable derived entries and makes the index larger; the prior review measured 2.19 MB to 7.41 MB at 5,000 user learnings. Mirroring still compares source/destination bytes; the zero-read guarantee applies to the index build, not every operation in pull.
Follow-up review: reproduced the executable attachment permission issue with a failing regression and fixed it; the final built-CLI read probe still passes after that correction.
Updated head:
8d6488299cf13c3e51b1ae384ea6d9824648da13. Rebased onto main6ebb31ce.