Skip to content

perf(chat): seek cached events by sequence - #4127

Merged
huangruiteng merged 2 commits into
loopx-project:mainfrom
Duang777:codex/optimize-core-hotpath-5
Sep 11, 2026
Merged

huangruiteng merged 2 commits into
loopx-project:mainfrom
Duang777:codex/optimize-core-hotpath-5

Conversation

@Duang777

@Duang777 Duang777 commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Use the monotonic Chat event sequence to binary-search the cached cursor position.
  • Avoid copying and scanning the full event cache on every SSE poll.
  • Preserve ordered tail results, including compacted caches with sequence gaps.
  • Stabilize loadLegacyCoordinationWriterFence read-failure reasons: strip the trailing absolute local path from Node fs error messages so the public reason matches the frozen parity-fixture contract and does not leak local machine paths. (commit 2fb24a89, legacy_writer_fence.ts:145-152)

Issue Or Task

Validation

  • 106 Chat and attached-session tests passed
  • uv run --no-project --with ruff ruff check loopx/chat_store.py tests/test_chat_event_cursor.py
  • scripts/loopx canary premerge --from-git-diff
  • git diff --check
  • Synthetic cached SSE benchmark: 100,000 events, cursor at 99,990; median 2.792 ms → 0.021 ms (~133× faster), peak 0.765 MiB → 0.003 MiB
  • tests/test_chat_event_cursor.py 1/1; fence caller-parity TS 22/22; shadow fence parity e2e 26/26; chat store input-validation + active-turn 32/32; remote CI 19 checks, 0 failures (verified on head 2fb24a89)

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring (no functional changes)
  • Documentation update
  • Test update

LoopX Area

  • Control plane (goals, todos, quota, scheduler, registry, runtime)
  • Benchmark boundary (adapters, runners, verifiers, scoring, evidence)
  • Capability or extension (providers, adapters, skills)
  • Public docs or presentation surface (README, protocols, dashboard)
  • Build, packaging, installer, or CI
  • Host or runtime integration

Technical Direction

  • Core control-plane hardening

  • Long-horizon benchmark evidence

  • Operator surface and IM integration

  • Shared Goal Authority and cross-host coordination

  • Architecture and research incubator

  • Target base branch: main

  • Direction tracker or promotion unit: N/A

Shared-authority RFC fixture impact

N/A. This change does not alter the TypeScript control-plane migration or shared Goal Authority fixtures.

Boundary Checklist

  • I did not commit .loopx/, .codex/goals/, live ACTIVE_GOAL_STATE.md, credentials, private benchmark traces, verifier output, raw agent sessions, internal document links, or local machine paths.
  • I did not duplicate maintainer-owned benchmark work unless a maintainer split out a public issue for it.
  • I kept the change scoped to the task above.
  • Every commit includes a DCO Signed-off-by trailer (git commit -s).

@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-5 branch 3 times, most recently from 8587ba9 to fb59325 Compare September 9, 2026 08:08
@Duang777

Duang777 commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator Author

@huangruiteng 当前 head 2fb24a8 已同步 main,并带入同一已验证的 Node 26 fence-error 稳定化修复。验证:业务回归 1/1、fence parity 22/22、Ruff 与 diff check 通过;新 CI 已触发,请按此 head 进行独立技术复审。

@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-5 branch 2 times, most recently from 73ee920 to 357a362 Compare September 10, 2026 01:42
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-5 branch from 357a362 to bbaccd4 Compare September 10, 2026 06:10
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
@Job28703

Copy link
Copy Markdown

Review — head 2fb24a89 (third pass)

Independent technical review on exact head 2fb24a891325b53f4191669f18e2240c5706dce5. The bisect core is byte-identical to the previously reviewed 73ee9208 content, so this pass focuses on the one new commit (2fb24a89 fence read-error stabilization) plus a full re-verification.

What changed

Core (unchanged from prior review): events_after replaces the per-poll full-cache linear scan + copy with bisect_right over sequence (loopx/chat_store.py:1346-1352). Correctness rests on a structural invariant — flush_events allocates sequence monotonically under the file lock (chat_store.py:1311-1315) over an append-only JSONL, so file order = ascending order. Cache hits now pass the list by reference (:1339-1341); flush swaps the reference ([*rows, *pending], :1318-1319) rather than mutating in place, so lock-free readers always see one consistent snapshot.

New in this head (legacy_writer_fence.ts:145-152): read-failure reason no longer leaks the absolute local path from Node fs error messages — error.path is extracted and reason.replace( '${path}', "") strips the trailing quoted path, aligning actual output with the frozen parity fixture contract (legacy_writer_fence_caller_parity_v0.json:136 expects "EISDIR: illegal operation on a directory, read" with no path). Non-fs errors without a string path keep the original message. reason_code is unchanged.

Verified locally on this head

  • tests/test_chat_event_cursor.py 1/1 — the NonIterableRows probe (any prefix iteration raises) mechanically proves the bisect path never scans the prefix, and gapped sequences (1/4/7) lock the post-compact "sorted but not dense" case
  • chat store input-validation + active-turn suites: 32/32
  • legacy_writer_fence_caller_parity.test.ts 22/22; shadow fence parity e2e: 26/26
  • Full TS control-plane suite: 908/909 (the single failure is the PostgreSQL integration test requiring a pg environment — unrelated to this PR)
  • Remote CI at review time: 19 checks, 0 failures

Non-blocking follow-ups

  1. No positive test for the path-strip itself. The parity fixture pins the path-free expectation, but nothing constructs a real ENOENT with a path and asserts the strip happened. If Node ever changes its message format, the strip silently stops working and paths leak back in. One focused unit test would close this.
  2. Monotonicity invariant is implicit. bisect correctness depends on the flush-lock allocation guarantee, but no comment at flush_events/_event_rows_locked states it. A future change to parallel allocation or history insertion would silently corrupt results. (Carried from prior reviews.)
  3. Minor: the fence fix is a different domain from the PR's chat-perf topic — fine, but a separate commit/PR would have kept review scopes cleaner.

Verdict

Textbook micro-optimization: the algorithm choice is exactly equivalent to the old filter semantics (sequence > after ≡ bisect_right right-open interval), the invariant it relies on is structurally guaranteed rather than assumed, and the new fence stabilization is defensive and correctly scoped. No blockers.

English verdict: APPROVE

@Job28703

Copy link
Copy Markdown

Follow-ups from the review above are now tracked: #4191 (path-strip positive test), #4192 (sequence-monotonicity invariant comment). The third note (fence fix vs. chat-perf mixing) is a one-off observation, not tracked.

@Job28703

Copy link
Copy Markdown

Reviewer note (PR body could not be edited by this account — leaving the delta here for the author/maintainer to fold in):

Summary — add one bullet:

Stabilize loadLegacyCoordinationWriterFence read-failure reasons: strip the trailing absolute local path from Node fs error messages so the public reason matches the frozen parity-fixture contract and does not leak local machine paths. (commit 2fb24a89, legacy_writer_fence.ts:145-152)

Issue Or Task — add:

Review follow-ups tracked in #4191 (positive unit test for the path-strip) and #4192 (sequence-monotonicity invariant comment for the bisect).

Validation — add (verified on head 2fb24a89):

  • tests/test_chat_event_cursor.py 1/1; fence caller-parity TS 22/22; shadow fence parity e2e 26/26; chat store input-validation + active-turn 32/32; remote CI 19 checks, 0 failures

Type of Change: consider checking Bug fix as well — the fence path-strip fixes a real reason-content/fixture-contract mismatch, not pure refactoring.

@Duang777

Copy link
Copy Markdown
Collaborator Author

@Job28703 已按建议更新 PR 描述:补充了 fence read-failure 稳定化摘要、#4191 / #4192 跟进项、head 2fb24a8 的验证结果,并勾选 Bug fix。感谢复审。

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

动机

这个 PR 优化聊天事件游标的缓存命中路径:调用方只需要某个 sequence 之后的事件,不应先复制整段缓存再线性扫描前缀。审阅绑定 exact head 2fb24a891325b53f4191669f18e2240c5706dce5。

改动思路

实现利用现有事件序列单调递增的不变量,对缓存中的 sequence 使用 bisect_right 定位边界,再只返回所需尾段;新增用不可迭代前缀覆盖“不能退化为线性扫描”,并用有间隙序列覆盖游标语义。附带的 legacy writer-fence 文本归一化不改变 reason code 或 fail-closed 权限语义,并且该部分已进入当前 main,所以合并后的有效增量主要是 chat cursor 优化。

具体改动

  • loopx/chat_store.py:缓存命中时由“复制全列表 + 线性过滤”改为二分定位后切片,保留非法游标、空结果和 sequence gap 行为。
  • tests/test_chat_event_cursor.py:新增 gapped sequence(1/4/7)与不可迭代前缀回归,证明不会扫描游标前的事件。
  • legacy_writer_fence.ts:从底层文件系统错误文本中去除本地路径;现有 22 行 caller-parity 已实际构造不可读目录并覆盖该正向路径。

关键代码讲解

flush_events 仍在文件锁内分配 sequence、追加 JSONL,并以新列表替换缓存引用;compaction 也保持顺序。因此 events_after 消费的是已排序序列,bisect_right 对“严格晚于 cursor”与原实现一致。独立基线/新实现矩阵覆盖 None、边界、间隙、越界和非法 cursor,共 10/10 等价。

对主干的风险

我在 exact head 上运行了 61 个 Python 测试和 22 个 TypeScript caller-parity 测试,另做了 10/10 基线等价矩阵;在最新 main 的合成合并树上又通过 61 个 Python 测试与 22 个 TypeScript 测试。Ruff、diff check 与远端必需检查均通过。

非阻塞 P2:二分查找依赖 sequence 单调递增,这个约束目前由写入结构保证但没有在生产者附近明示。建议后续在 sequence 分配或二分调用旁补一条简洁不变量注释;已有 #4192 跟踪,无需为本 PR 增加运行时断言或新框架。

我的整体评价

这是一个边界清楚、行为保持且有真实性能回归覆盖的优化。默认行为、权限语义、错误分类和游标兼容性均未被悄然改变;相关 future-facing 检查已做,除已登记的不变量注释外不需要扩大范围。结论:APPROVE。

English verdict: APPROVE for exact head 2fb24a891325b53f4191669f18e2240c5706dce5. The cache-hit cursor path now seeks by monotonic sequence and returns only the required tail, with exact-head and current-main integration coverage. The only follow-up is a non-blocking P2 to document the sorted-sequence invariant already tracked in #4192.

@huangruiteng
huangruiteng merged commit 81a7d2d into loopx-project:main Sep 11, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants