Skip to content

perf: retrieval, import, LangChain, and session-context optimizations - #3569

Merged
qin-ctx merged 9 commits into
volcengine:mainfrom
huangxun375-stack:perf/ov0616-main-split
Jul 30, 2026
Merged

perf: retrieval, import, LangChain, and session-context optimizations#3569
qin-ctx merged 9 commits into
volcengine:mainfrom
huangxun375-stack:perf/ov0616-main-split

Conversation

@huangxun375-stack

@huangxun375-stack huangxun375-stack commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

This PR reduces event-loop blocking and redundant I/O on hot retrieval, import, LangChain session, and session-context paths under concurrent load. It is rebased onto current main, including RFC #3330 session recovery and #3575 async recording.

The parse, retrieve, and LangChain changes are conservative: their knobs are opt-in or preserve existing behavior when unset. The session-context change is different — it changes default behavior with no opt-out, and deliberately deviates from RFC #3330 in two places. See "Behavioral change: session context assembly" below.

Human Involvement

  • A human participated in the implementation or review loop
  • This PR was generated entirely by AI agents without human participation in the loop

Related Issue

N/A

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Motivation (before → after)

Measured on current main vs this branch, same host, same corpus, single round.

Path main This PR
Search, 10 concurrent (p50 / p95 / avg) 9276 / 11083 / 9298 ms 634 / 839 / 666 ms
Excel import, ~945 KiB, wait=false, 16 concurrent batch wall time 21.322 s 3.449 s
get_session_context archive reads, session with N archives O(N) markers + overviews + .meta.json O(1) — one archive touched
LangChain get_session calls per turn for commit policy 1 0

Reproduction notes:

  • Search: the PR arm runs with enable_intent=false. Typed queries per request drop from 3–5 to 1, so this measures the PR plus that config, not a single-variable algorithm comparison. Do not read it as a like-for-like speedup with all features on.
  • Excel: the PR arm needs excel.enable_process_pool. Without it the two arms are equivalent — our first attempt measured 17.975 s vs 18.531 s because the gate was off, and that run was discarded.
  • Excel number is pending re-measurement. It was originally produced with both the process pool and a Markdown fast-write path. Fast-write has since been removed (see below), and telemetry attributes only ~135 ms of the ~17.9 s gain to it, so we expect ~3.5–3.6 s. We will update this row with a re-measured figure.
  • Absolute values depend on model endpoints, corpus size, and storage; the relative shape is what we expect reviewers to reproduce.
  • Historical figures from older versions and environments (88.2s → 11.6s / 4.8s) are not comparable to this table and have been removed to avoid mixing baselines.

Enabling the parse optimization

Top-level excel config — note this is not parsers.excel; a wrong key fails startup:

"excel": {
  "enable_process_pool": true,
  "process_pool_workers": 8
}

Confirm the gate is active in the log before trusting any measurement:

[ExcelParserProcessPool] Started process pool workers=8

Changes Made

  • perf(parse): optional Excel process-pool offload, and skip image scanning when a layout has no local image references.
  • perf(retrieve): add a retrieval.enable_intent switch (default true, current behavior).
    When off, search skips session load, get_context_for_search, and
    IntentAnalyzer, and runs the raw query on the same path as a no-session search.
  • perf(langchain): create sessions only on NOT_FOUND; reuse write-returned pending tokens for commit policy on both the sync and async paths.
  • fix(session): return pending_tokens from the REST and LocalClient write responses so the above optimization actually applies to real clients.
  • perf(session): stop the context scan at the newest terminal archive. See below.

Behavioral change: session context assembly

Why

Archive history grows without bound, but get_session_context walked all of it on every call: one marker read per archive, an overview read per Working-Memory archive, and a .meta.json read per completed archive for checkpoints. On long-lived sessions this dominates the call.

What changed

The scan goes newest → oldest and stops at the first terminal marker (.done or .failed.json). Nothing at or older than that terminal is read.

Newest terminal Overview Messages
.done, overview readable that overview newer non-terminal raw + root live
.done, overview missing/unreadable none, warning logged newer non-terminal raw + root live
.failed.json none newer non-terminal raw + root live
no terminal yet none all archive raw + root live

A session with 100 archives now touches exactly one archive directory.

Deviations from RFC #3330 — please review explicitly

These are intentional and accepted to make the read path independent of history length. We are not claiming RFC parity.

  1. An uncovered failed archive no longer replays its raw messages into get_session_context. RFC [RFC] OpenViking Session 上下文优化:Turn-aware Retention、Pending/Failed 恢复与预算控制 #3330 defines logical live = uncovered pending raw + uncovered failed raw + root live; this PR drops the failed-raw term. The raw file stays durable, and Phase 2 roll-forward (_prepare_phase2_archive_messages) still replays uncovered failed archives into the next archive's input, so those messages remain reachable and are still compressed into a later overview. The gap is the window between the failure and the next successful archive.

  2. Only the newest terminal archive's checkpoints are restored. When one long User Turn is partially committed more than once, earlier disjoint prefixes are no longer merged into the anchor's checkpoint. Restoring them requires reading .meta.json for an unbounded number of archives.

  3. stats.failedArchives is now 0/1 ("is the newest terminal a failure") rather than a count of uncovered failed archives, because an exact count needs the full marker scan this change removes. stats.totalArchives stays exact — it comes from the directory listing.

We are happy to gate item 1 behind a config flag or adopt a different recovery semantic. A bounded alternative we considered but did not implement: prune using the newest completed archive's coverage_start_archive, which would keep the common case at O(1) without dropping the failed-raw term. We did not pursue it because it reaches further into RFC #3330 than we were comfortable changing unilaterally. Guidance from the RFC author on items 1 and 2 would help.

What is unchanged

  • _scan_archive_states() still performs a full scan and remains the source of truth for Phase 2 waiting, the coverage frontier, and roll-forward. Memory extraction is unaffected.
  • Phase 2 .done coverage metadata (coverage_start_archive, coverage_end_archive, covered_failed_archives) is written exactly as before.
  • Checkpoint generation in Phase 2 is unchanged; only restore-side breadth changed.
  • Message assembly still uses stable dedup by message.id; pre_archive_abstracts still returns [].

Removed since the first review round

The Markdown fast-write path (OPENVIKING_MARKDOWN_APPLY_FAST_WRITE) has been dropped. It replaced _write_section with a raw viking_fs.write, which silently bypassed _ensure_parent_dirs and _run_with_encrypted_write_lock. That was an oversight during a path substitution, not a decision that the lock was unnecessary. Telemetry puts its share of the 16-way Excel batch at roughly 135 ms out of a 17.9 s total, so the correctness risk was not worth the gain.

Section writes now go back through write_file, and OPENVIKING_MARKDOWN_APPLY_CONCURRENCY is gone with the path it gated. OPENVIKING_MARKDOWN_APPLY_PROFILE remains as a pure logging switch. The Excel process pool, which accounts for the rest of the import speedup, is unchanged. A regression test now asserts section writes do not bypass write_file.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Post-rebase on Linux:

Suite Result
tests/integration/langchain_langgraph/test_async_recording.py (from #3575) 13/13
tests/unit/test_langchain_integration.py + async 107/107
tests/session/test_session_context.py 21/21
tests/server/test_api_sessions.py 32/32
tests/parse/test_excel_process_pool.py + test_markdown_apply_layout.py 18/18
tests/session/test_session_retention_integration.py 37/38

The one failure, test_stale_worker_uses_lock_snapshot_memory_policy_for_queue_message, reproduces identically without this branch and is unrelated. Full-suite runs of tests/session/ and tests/parse/ were compared before and after these changes; the sets of failing tests are identical.

Notable new coverage:

  • test_get_session_context_does_not_touch_archives_older_than_terminal — no marker, overview, .meta.json, or messages read may reference an archive older than the terminal.
  • test_write_responses_return_pending_tokens_for_commit_policy — drives the real REST endpoints, and asserts the write-returned value equals what get_session would report.
  • test_async_assemble_skips_create_for_existing_session / ..._creates_session_only_on_not_found / test_async_history_does_not_create_session_on_non_not_found_error — async parity and error semantics.
  • test_registry_resolves_excel_against_markdown — the inner MarkdownParser really receives inherited sectioning values.
  • test_sections_go_through_write_section_not_raw_write — guards against reintroducing a lock-bypassing write path.
  • Deviation pins: test_context_stops_at_newest_terminal_without_replaying_older_failed_raw, test_repeated_partial_commits_restore_only_newest_checkpoint, and durability assertions in test_queue_enqueue_failure_marks_archive_failed_and_keeps_raw_durable / test_phase1_root_rewrite_failure_marks_orphan_archive_failed.

Suggested focused re-checks:

  1. Parse: 16-way Excel addResource batch wall time, pool on/off, with the gate log line present.
  2. Retrieve: c10 search latency with default intent, then intent off.
  3. LangChain: one turn should be get_session_context → (create only if NOT_FOUND) → batch_add_messages → commit, with no extra get_session. Worth checking on both the sync and async entry points.
  4. Session context: multi-archive session — overview equals the newest completed terminal only; a newest .failed yields an empty overview; archives older than the terminal are not read.

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Documentation is not updated yet. New or changed surfaces needing docs: excel.enable_process_pool / excel.process_pool_workers and their inheritance from parsers.markdown, retrieval.enable_intent, pending_tokens on write responses, and the get_session_context / latest_archive_overview / stats.failedArchives semantics above. Happy to add these here or in a follow-up, whichever maintainers prefer.

Screenshots (if applicable)

N/A — performance tables above.

Additional Notes

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)

Co-authored-by:
ceppetellilines-dot ningshaopeng1990@cmbchina.com
Eurekaxun eurekaxun@163.com

@qin-ctx qin-ctx 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.

本次 review 发现 3 个 blocking 问题:archive 失败后的上下文恢复与 RFC/公开语义不一致,Excel 新配置会静默改变既有导入行为,LangChain 的 pending_tokens 优化未接入真实 client/server 返回契约。另有 2 个 non-blocking 设计问题,涉及异步路径的一致性和 working-memory archive 的实际 I/O 范围。具体触发条件、执行过程和影响见 inline comments。

Comment thread openviking/session/session.py Outdated
Comment thread openviking/parse/registry.py Outdated
Comment thread openviking/integrations/langchain/recording.py
Comment thread openviking/integrations/langchain/context.py
Comment thread openviking/session/session.py
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
…nfig

Address review on PR volcengine#3569.

Remove the OPENVIKING_MARKDOWN_APPLY_FAST_WRITE path. It replaced
_write_section with a raw viking_fs.write, which silently bypassed
_ensure_parent_dirs and _run_with_encrypted_write_lock. Telemetry puts its
share of the 16-way Excel batch at roughly 135ms out of a 17.9s total, so the
correctness risk was not worth the gain. Section writes go back through
write_file; OPENVIKING_MARKDOWN_APPLY_CONCURRENCY is gone with the path it
gated, and the profile log stays. The Excel process pool, which accounts for
the rest of that speedup, is unchanged.

Keep Excel sectioning following parsers.markdown when parsers.excel does not
set it. ExcelParser converts to Markdown and delegates sectioning to an inner
MarkdownParser, so pointing the registry at the new config section would have
silently changed section boundaries, node structure and stable Viking URIs for
deployments that had tuned parsers.markdown. Explicit parsers.excel values
still win, and the process-pool knobs are never inherited.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
Address review on PR volcengine#3569.

Archive history grows without bound, but get_session_context walked all of it:
one marker read per archive, an overview read per Working-Memory archive, and a
.meta.json read per completed archive for checkpoints. The previous commit only
moved the overview cut-off, so the message and checkpoint paths still scanned
everything and the intended saving did not materialize.

Scan newest to oldest and stop at the first terminal marker. Nothing at or older
than that terminal is read: overview comes from the terminal archive when it is
completed and readable, raw messages come only from newer non-terminal archives,
and checkpoints come only from the terminal archive. A session with 100 archives
now touches exactly one.

Deliberate deviations from RFC volcengine#3330, kept narrow and confined to this read path:

- An uncovered failed archive no longer replays its raw messages into
  get_session_context, dropping the failed-raw term from logical live. The raw
  file stays durable and Phase 2 roll-forward still absorbs it into a later
  overview; the gap is the window between the failure and the next successful
  archive.
- Only the newest terminal archive's checkpoints are restored, so a long User
  Turn committed partially more than once keeps just its newest compressed
  prefix.
- stats.failedArchives becomes 0/1 ("is the newest terminal a failure") because
  an exact count needs the full marker scan this change removes.
  stats.totalArchives stays exact via the directory listing.

Memory extraction is unaffected: _scan_archive_states still performs a full scan
and remains the source of truth for Phase 2 waiting, the coverage frontier and
roll-forward. Marker probing uses exists() rather than read_file exception
handling. Tests cover the terminal-stop cost bound, the deviations above, and
that raw files stay durable while the read path stops.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
Address review on PR volcengine#3569.

The commit-policy optimization read pending_tokens from the write response, but
no real producer supplied it: the REST add_message and batch_add_messages
handlers returned only session_id/message_count/added, and LocalClient matched
them. Only the LangChain in-memory test double returned the field, so
persisted_pending_tokens was always None on real deployments,
apply_commit_policy always fell back to get_session, and the fake-based tests
reported an optimization that never took effect in production.

Return the post-write pending_tokens from both REST handlers and both
LocalClient methods. The value is already maintained in O(1) inside
_append_messages, so it is exact at the point the write returns, which is what
the commit policy needs. Reading it goes through a helper that degrades to 0
when a session object does not expose meta, keeping lightweight and legacy
session implementations working.

Also update the REST failed-archive context test for the terminal-stop read
path, and add coverage that exercises the real endpoints rather than a test
double: the write-returned value must be positive, grow across writes, and match
what get_session would have reported, which is exactly the round trip this
field removes.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
Address review on PR volcengine#3569.

The previous commit changed only the synchronous path, so LangGraph callers,
which use the async entry points, kept paying for every optimization it claimed:

- aassemble still called _aensure_session unconditionally before each read, so
  an existing session took an extra create_session per turn.
- _aget_session_context had no NOT_FOUND branch at all; its correctness relied
  on that unconditional ensure, so removing the ensure alone would have left
  recall pointing at a session that was never created.
- aget_messages created a session after any exception, while the sync path only
  did so on NOT_FOUND. A 5xx or a timeout therefore added a create_session call
  during exactly the incidents when the service was already struggling.

Read first and create only on NOT_FOUND, mirroring the sync path, and add the
missing ensure branches to _aget_session_context so the first use still
materializes an empty session without a second context read. Tests assert that
repeated assembles on an existing session issue no create_session, that a
missing session issues exactly one, and that a non-NOT_FOUND failure issues
none.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
Run Excel-to-Markdown conversion and layout in a process pool gated by
excel.enable_process_pool / excel.process_pool_workers, add a Markdown
fast-write layout path, and skip image scanning when a layout has no
local image refs. Includes unit tests for each path.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
…nfig

Address review on PR volcengine#3569.

Remove the OPENVIKING_MARKDOWN_APPLY_FAST_WRITE path. It replaced
_write_section with a raw viking_fs.write, which silently bypassed
_ensure_parent_dirs and _run_with_encrypted_write_lock. Telemetry puts its
share of the 16-way Excel batch at roughly 135ms out of a 17.9s total, so the
correctness risk was not worth the gain. Section writes go back through
write_file; OPENVIKING_MARKDOWN_APPLY_CONCURRENCY is gone with the path it
gated, and the profile log stays. The Excel process pool, which accounts for
the rest of that speedup, is unchanged.

Keep Excel sectioning following parsers.markdown when parsers.excel does not
set it. ExcelParser converts to Markdown and delegates sectioning to an inner
MarkdownParser, so pointing the registry at the new config section would have
silently changed section boundaries, node structure and stable Viking URIs for
deployments that had tuned parsers.markdown. Explicit parsers.excel values
still win, and the process-pool knobs are never inherited.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
Address review on PR volcengine#3569.

Archive history grows without bound, but get_session_context walked all of it:
one marker read per archive, an overview read per Working-Memory archive, and a
.meta.json read per completed archive for checkpoints. The previous commit only
moved the overview cut-off, so the message and checkpoint paths still scanned
everything and the intended saving did not materialize.

Scan newest to oldest and stop at the first terminal marker. Nothing at or older
than that terminal is read: overview comes from the terminal archive when it is
completed and readable, raw messages come only from newer non-terminal archives,
and checkpoints come only from the terminal archive. A session with 100 archives
now touches exactly one.

Deliberate deviations from RFC volcengine#3330, kept narrow and confined to this read path:

- An uncovered failed archive no longer replays its raw messages into
  get_session_context, dropping the failed-raw term from logical live. The raw
  file stays durable and Phase 2 roll-forward still absorbs it into a later
  overview; the gap is the window between the failure and the next successful
  archive.
- Only the newest terminal archive's checkpoints are restored, so a long User
  Turn committed partially more than once keeps just its newest compressed
  prefix.
- stats.failedArchives becomes 0/1 ("is the newest terminal a failure") because
  an exact count needs the full marker scan this change removes.
  stats.totalArchives stays exact via the directory listing.

Memory extraction is unaffected: _scan_archive_states still performs a full scan
and remains the source of truth for Phase 2 waiting, the coverage frontier and
roll-forward. Marker probing uses exists() rather than read_file exception
handling. Tests cover the terminal-stop cost bound, the deviations above, and
that raw files stay durable while the read path stops.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
Address review on PR volcengine#3569.

The commit-policy optimization read pending_tokens from the write response, but
no real producer supplied it: the REST add_message and batch_add_messages
handlers returned only session_id/message_count/added, and LocalClient matched
them. Only the LangChain in-memory test double returned the field, so
persisted_pending_tokens was always None on real deployments,
apply_commit_policy always fell back to get_session, and the fake-based tests
reported an optimization that never took effect in production.

Return the post-write pending_tokens from both REST handlers and both
LocalClient methods. The value is already maintained in O(1) inside
_append_messages, so it is exact at the point the write returns, which is what
the commit policy needs. Reading it goes through a helper that degrades to 0
when a session object does not expose meta, keeping lightweight and legacy
session implementations working.

Also update the REST failed-archive context test for the terminal-stop read
path, and add coverage that exercises the real endpoints rather than a test
double: the write-returned value must be positive, grow across writes, and match
what get_session would have reported, which is exactly the round trip this
field removes.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
Address review on PR volcengine#3569.

The previous commit changed only the synchronous path, so LangGraph callers,
which use the async entry points, kept paying for every optimization it claimed:

- aassemble still called _aensure_session unconditionally before each read, so
  an existing session took an extra create_session per turn.
- _aget_session_context had no NOT_FOUND branch at all; its correctness relied
  on that unconditional ensure, so removing the ensure alone would have left
  recall pointing at a session that was never created.
- aget_messages created a session after any exception, while the sync path only
  did so on NOT_FOUND. A 5xx or a timeout therefore added a create_session call
  during exactly the incidents when the service was already struggling.

Read first and create only on NOT_FOUND, mirroring the sync path, and add the
missing ensure branches to _aget_session_context so the first use still
materializes an empty session without a second context read. Tests assert that
repeated assembles on an existing session issue no create_session, that a
missing session issues exactly one, and that a non-NOT_FOUND failure issues
none.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
ceppetellilines-dot and others added 7 commits July 29, 2026 17:04
Create sessions only after context/history NOT_FOUND (including code-based
detection), and prefer batch/add write-returned pending_tokens for commit
policy with legacy get_session fallback. Surface pending_tokens on in-memory
test client add/batch responses.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
Walk archives newest-to-oldest and stop at the first completed/failed
terminal: inject overview only for completed; on failed stop without
overview and never fall back to older archives. Keep RFC volcengine#3330
coverage/uncovered/checkpoint message assembly; skip unused abstract
reads. Lazy-load overview for non-WM completed markers during scan.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
…nfig

Address review on PR volcengine#3569.

Remove the OPENVIKING_MARKDOWN_APPLY_FAST_WRITE path. It replaced
_write_section with a raw viking_fs.write, which silently bypassed
_ensure_parent_dirs and _run_with_encrypted_write_lock. Telemetry puts its
share of the 16-way Excel batch at roughly 135ms out of a 17.9s total, so the
correctness risk was not worth the gain. Section writes go back through
write_file; OPENVIKING_MARKDOWN_APPLY_CONCURRENCY is gone with the path it
gated, and the profile log stays. The Excel process pool, which accounts for
the rest of that speedup, is unchanged.

Keep Excel sectioning following parsers.markdown when parsers.excel does not
set it. ExcelParser converts to Markdown and delegates sectioning to an inner
MarkdownParser, so pointing the registry at the new config section would have
silently changed section boundaries, node structure and stable Viking URIs for
deployments that had tuned parsers.markdown. Explicit parsers.excel values
still win, and the process-pool knobs are never inherited.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
Address review on PR volcengine#3569.

Archive history grows without bound, but get_session_context walked all of it:
one marker read per archive, an overview read per Working-Memory archive, and a
.meta.json read per completed archive for checkpoints. The previous commit only
moved the overview cut-off, so the message and checkpoint paths still scanned
everything and the intended saving did not materialize.

Scan newest to oldest and stop at the first terminal marker. Nothing at or older
than that terminal is read: overview comes from the terminal archive when it is
completed and readable, raw messages come only from newer non-terminal archives,
and checkpoints come only from the terminal archive. A session with 100 archives
now touches exactly one.

Deliberate deviations from RFC volcengine#3330, kept narrow and confined to this read path:

- An uncovered failed archive no longer replays its raw messages into
  get_session_context, dropping the failed-raw term from logical live. The raw
  file stays durable and Phase 2 roll-forward still absorbs it into a later
  overview; the gap is the window between the failure and the next successful
  archive.
- Only the newest terminal archive's checkpoints are restored, so a long User
  Turn committed partially more than once keeps just its newest compressed
  prefix.
- stats.failedArchives becomes 0/1 ("is the newest terminal a failure") because
  an exact count needs the full marker scan this change removes.
  stats.totalArchives stays exact via the directory listing.

Memory extraction is unaffected: _scan_archive_states still performs a full scan
and remains the source of truth for Phase 2 waiting, the coverage frontier and
roll-forward. Marker probing uses exists() rather than read_file exception
handling. Tests cover the terminal-stop cost bound, the deviations above, and
that raw files stay durable while the read path stops.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
Address review on PR volcengine#3569.

The commit-policy optimization read pending_tokens from the write response, but
no real producer supplied it: the REST add_message and batch_add_messages
handlers returned only session_id/message_count/added, and LocalClient matched
them. Only the LangChain in-memory test double returned the field, so
persisted_pending_tokens was always None on real deployments,
apply_commit_policy always fell back to get_session, and the fake-based tests
reported an optimization that never took effect in production.

Return the post-write pending_tokens from both REST handlers and both
LocalClient methods. The value is already maintained in O(1) inside
_append_messages, so it is exact at the point the write returns, which is what
the commit policy needs. Reading it goes through a helper that degrades to 0
when a session object does not expose meta, keeping lightweight and legacy
session implementations working.

Also update the REST failed-archive context test for the terminal-stop read
path, and add coverage that exercises the real endpoints rather than a test
double: the write-returned value must be positive, grow across writes, and match
what get_session would have reported, which is exactly the round trip this
field removes.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
Address review on PR volcengine#3569.

The previous commit changed only the synchronous path, so LangGraph callers,
which use the async entry points, kept paying for every optimization it claimed:

- aassemble still called _aensure_session unconditionally before each read, so
  an existing session took an extra create_session per turn.
- _aget_session_context had no NOT_FOUND branch at all; its correctness relied
  on that unconditional ensure, so removing the ensure alone would have left
  recall pointing at a session that was never created.
- aget_messages created a session after any exception, while the sync path only
  did so on NOT_FOUND. A 5xx or a timeout therefore added a create_session call
  during exactly the incidents when the service was already struggling.

Read first and create only on NOT_FOUND, mirroring the sync path, and add the
missing ensure branches to _aget_session_context so the first use still
materializes an empty session without a second context read. Tests assert that
repeated assembles on an existing session issue no create_session, that a
missing session issues exactly one, and that a non-NOT_FOUND failure issues
none.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
Add a `retrieval.enable_intent` config switch, default true, matching current
behavior. When it is false, search() skips session.load,
get_context_for_search, and IntentAnalyzer, and searches with the raw query on
the same path as a no-session search. Session scanning is skipped at all three
entry points (local client, REST, MCP) via SearchService.is_intent_enabled, so
a disabled intent path does not pay for a session load whose result VikingFS
would ignore.

Intent analysis fans a single request out into several typed queries, each with
its own vector search. On a 10-concurrent search benchmark, turning it off took
typed queries per request from 3-5 down to 1.

The rerank changes that were originally part of this commit have been dropped:
maintainers indicated rerank is likely to be refactored, so those are better
submitted separately against the new structure. This commit no longer touches
hierarchical_retriever.py, openai_rerank.py, or the rerank config.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>

@qin-ctx qin-ctx 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.

本轮基于当前 Head c0b87a9 复查。前一轮提出的 pending_tokens 真实返回契约、LangChain 异步路径一致性和历史 overview O(N) 读取问题已经处理;Excel 对 Markdown 分段配置的默认继承也已补齐。当前仍有 3 个需要合并前解决的问题:最新 terminal 失败时已接受的 overview 与 failed raw 都会从 Session Context 消失;只读取最新 terminal checkpoint 会丢失同一 Turn 的较早压缩前缀;Excel 显式设置为类默认值时会被误判为未配置并被 Markdown 值覆盖。具体触发条件和影响见 inline comments。

Comment thread openviking/session/session.py
Comment thread openviking/session/session.py
Comment thread openviking_cli/utils/config/parser_config.py Outdated
huangxun375-stack pushed a commit to huangxun375-stack/OpenViking that referenced this pull request Jul 29, 2026
…ault

Address review on PR volcengine#3569.

`with_sectioning_defaults_from` decided whether a field was unset by comparing
it against the class default. That cannot distinguish "the key was absent" from
"the user wrote a value that happens to equal the default", so a config like

    parsers.markdown.max_section_size: 512
    parsers.excel.max_section_size: 2048

resolved Excel to 512 — the documented "explicit parsers.excel values always
win" contract was false for exactly the case where it matters, and there was no
way to express "Markdown at 512, Excel deliberately at 2048".

`ExcelConfig.from_dict` now records which keys were present, and inheritance
skips those. The field is excluded from equality and repr so two configs with
the same values remain equal regardless of how they were built.

Two consequences worth noting:

- A config built directly, without `from_dict`, carries no key provenance and is
  treated as fully explicit, so a hand-constructed `ExcelConfig` is never
  silently rewritten.
- `OpenVikingConfig.excel` therefore defaults via `ExcelConfig.from_dict({})`
  rather than the bare constructor. Without that, a deployment with no
  `parsers.excel` section at all would have been read as fully explicit and
  would have stopped following `parsers.markdown` — the exact compatibility case
  this inheritance exists to preserve.

Tests pin the reviewer's case, both full-config paths (absent section inherits,
explicit section wins), provenance-independent equality, and the
hand-constructed behavior.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
…ault

Address review on PR volcengine#3569.

`with_sectioning_defaults_from` decided whether a field was unset by comparing
it against the class default. That cannot distinguish "the key was absent" from
"the user wrote a value that happens to equal the default", so a config like

    parsers.markdown.max_section_size: 512
    parsers.excel.max_section_size: 2048

resolved Excel to 512. The documented "explicit parsers.excel values always win"
contract was false for exactly the case where it matters, and there was no way
to express "Markdown at 512, Excel deliberately at 2048".

`ExcelConfig` now records which keys a config source actually provided, and
inheritance skips those. Provenance is a plain instance attribute reached via
`with_explicit_keys` / `explicit_keys` rather than a dataclass field, which
matters for three reasons found while implementing it:

- A dataclass field appears in `dataclasses.asdict()` and `model_dump()` output,
  and being a frozenset it makes those results non-JSON-serializable for
  callers.
- A dataclass field is a legal config key, so a config file could forge
  provenance and steer the inheritance decision.
- Equality must ignore it, so two configs with the same values stay equal
  regardless of how they were built.

Both parser-config entry points now route through `from_dict` even with no data.
`get_parser_config` and `load_parser_configs_from_dict` previously used a bare
constructor for an absent section, and `OpenVikingConfig.excel` defaulted the
same way; provenance would then be unknown and an absent `parsers.excel` section
would stop inheriting from `parsers.markdown` — the compatibility case this
inheritance exists to preserve.

A config built directly, without `from_dict`, carries no key information and is
treated as fully explicit, so a hand-constructed `ExcelConfig` is never silently
rewritten.

Tests cover the reviewer's case, both full-config paths (absent section
inherits, explicit section wins), inheritance through the parser loader,
serialization cleanliness, forged-provenance rejection, and preservation across
replace/copy/deepcopy.

Organizational contribution: Information Technology Department of China Merchants Bank (招商银行信息技术部)
Co-authored-by: Eurekaxun <eurekaxun@163.com>
@qin-ctx
qin-ctx merged commit 44c6df2 into volcengine:main Jul 30, 2026
4 of 5 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in OpenViking project Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants