fix(session): bound _wait_for_previous_archive_done to prevent permanent SessionCommit queue stall - #3713
fix(session): bound _wait_for_previous_archive_done to prevent permanent SessionCommit queue stall#3713ZaynJarvis wants to merge 1 commit into
Conversation
…ent SessionCommit queue stall Phase 2 of a session commit waits for all earlier archives of the same session to reach a terminal state before starting. Since #3380 that wait is an unbounded while-True poll loop: if an earlier archive is orphaned (Phase 1 finished with status=ready but its queue item was lost to a crash, upgrade, or historical enqueue bug), the wait never returns. Each stuck commit permanently occupies one worker semaphore slot before tracker.start() is ever reached, so once max_concurrent such commits accumulate the whole session_commit queue freezes: everything pending, nothing running, zero errors, and restarts do not help. Bound the wait with a timeout (default 30 min, matching the existing _PHASE2_QUEUE_WAIT_TIMEOUT_SECONDS). On expiry, mark the still-pending predecessors terminally failed via the existing .failed.json marker (so their raw messages are replayed into a later commit by the established _prepare_phase2_archive_messages roll-forward) and raise TimeoutError so the current task is evicted to failed state through the normal failure path: .failed.json for the current archive, tracker.fail, queue ack, and a released worker slot. No data is silently dropped and the next commit proceeds normally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
t0saki
left a comment
There was a problem hiding this comment.
Thanks for addressing the global SessionCommit queue-stall failure mode. I reviewed head 75ce8c4f, including the complete changed files and the call chain from Phase 1 enqueue through QueueFS processing, archive-state scanning, Phase 2 extraction, marker publication, and task finalization.
The bounded wait does release a worker when a predecessor never becomes terminal, and the focused orphan-timeout test passes. My recommendation is to request changes, because the current recovery transition has two blocking lifecycle gaps:
- A waiter's local timeout is used as proof that every pending predecessor has lost its owner. The inspected decision path checks archive markers and Phase 1 readiness, but not QueueFS/TaskWorkIndex ownership or an expired heartbeat/lease, so it can terminally fail work that is still legitimately running.
- When a predecessor really is orphaned, the code changes its archive marker to failed but only fails the current waiting task. The orphaned predecessor's own task record is not reconciled and can remain pending/running indefinitely.
Evidence boundaries: I ran the real _wait_for_previous_archive_done() implementation in focused harnesses and used the real TaskTracker for the task-state reproduction. I did not run a real four-worker QueueManager concurrency reproduction, and I do not have production Phase 2 duration samples. The concurrent duplicate-execution consequence in the first comment is therefore explicitly identified as an inference from the call chain, not a measured production observation.
Focused validation: the new orphan-timeout test and the existing normal-wait test both pass; git diff --check passes and the checkout remains clean.
| timeout, | ||
| archive_index, | ||
| ) | ||
| await self._write_failed_marker( |
There was a problem hiding this comment.
[Bug] (blocking)
The timeout treats "still pending when this waiter's local 1,800-second deadline expires" as proof that every predecessor's QueueFS owner is gone, but those states are not equivalent.
I traced the decision domain through _wait_for_previous_archive_done(), _scan_archive_states(), the four-way SessionCommit worker, _run_memory_extraction(), and TaskWorkIndex. At this point the code has checked archive markers and phase1.status; it has not checked whether the predecessor queue item or asyncio task is still active, nor an expired heartbeat or lease. The code path permits a legitimate predecessor to remain active past a later waiter's 1,800-second deadline: extraction runs before a separate wait_for_request(..., timeout=1800) wait, and there is no overall Phase 2 deadline that proves the owner dead.
Measured in a focused harness: I invoked the real PR-head implementation of _wait_for_previous_archive_done() with archives 001–003 reported as pending and recorded its marker writes. When archive 004's deadline elapsed, this branch wrote phase2_wait_timeout to all three predecessors. The added test covers only a predecessor with no simulated live owner.
Inferred from the call chain, not observed with a real concurrent QueueManager: if one of those predecessors is still executing, it does not re-check .failed.json after passing this wait and can later write .done. Before that happens, another commit can observe the failed marker and roll the same raw messages forward. That creates a window for contradictory terminal markers and duplicate concurrent extraction.
I do not have production duration measurements and did not run a real four-worker concurrency reproduction. The correctness issue does not depend on this being common, though: a timeout by itself is not an owner-death proof. Please gate predecessor mutation on an authoritative liveness signal—for example, an archive-to-task/work mapping combined with absent QueueFS/TaskWorkIndex ownership, or an expired heartbeat/lease. Without such a signal, the timeout should fail only the waiter rather than rewriting predecessor state.
| "its Phase 2 queue item is presumed lost" | ||
| ), | ||
| ) | ||
| raise TimeoutError( |
There was a problem hiding this comment.
[Bug] (blocking)
When a predecessor really is orphaned, this path changes the archive marker but does not terminate that predecessor's own task record.
I traced the lifecycle domain through the persisted Phase 1 queue_message, this timeout branch, _run_memory_extraction()'s exception handler, and TaskTracker. The predecessor's task ID is available in its Phase 1 metadata, but this helper only writes the predecessor's .failed.json and raises. The exception handler then calls tracker.fail(task_id, ...) with the current waiting archive's task ID. There is no corresponding transition for the predecessor task that was just declared lost.
Concrete example: archive 001's queue item is lost and archive 002 times out waiting for it. The filesystem reports archive 001 as failed, while the task API can still report task 001 as pending/running; only task 002 becomes failed. Active task states have no TTL-based cleanup, so this split can persist indefinitely even though the queue slot was released.
Measured in a focused runtime harness: using the real timeout method and a real TaskTracker, the resulting statuses were archive_001: pending and archive_002: failed. This harness did not run a real QueueManager, but the task transition itself follows the production exception path directly.
Once predecessor ownership has been authoritatively determined to be lost, please reconcile its persisted task ID as failed as part of the same recovery transition, and add an integration assertion for both truths: the predecessor archive is failed and its original task record is terminal. Otherwise this PR unblocks queue consumption but leaves the orphan task permanently non-terminal.
出了什么事
线上一台 OpenViking(0.4.12.dev86)的 SessionCommit 队列整个冻住了 10 天以上:860 条任务全部 pending,4 条 in-progress(正好是 worker 并发上限),0 条完成,0 条报错。查询、检索都正常,只有会话记忆提取完全停摆。
根因
#3380(0ab85f45,7-24 合入)给 commit 加了一个"等同一 session 更早的 archive 先完成"的循环(
_wait_for_previous_archive_done),但这个循环没有超时。如果某个 archive 的队列消息丢了(进程崩溃、升级、老版本存量数据都可能造成),它永远不会完成,等它的 commit 就永远卡住。卡住时占着 worker 的并发槽,4 个槽占满后整个队列不再消费任何任务——一个脏 session 就能冻住所有 session。v0.4.12 是第一个带这个问题的 release。
怎么修的
_ARCHIVE_WAIT_TIMEOUT_SECONDS,风格对齐现有的_PHASE2_QUEUE_WAIT_TIMEOUT_SECONDS),循环内原有的轮询 reconcile 保留,作为重试。.failed.json并打 error 日志,当前任务抛TimeoutError,走既有异常路径落成 failed(tracker.fail + 消息 ack + 释放槽位)。任务只会变 failed,不会再卡 pending。测试
新增
test_phase2_wait_times_out_and_fails_orphaned_pending_archive:构造一个永不完成的前置 archive,验证不挂死、前置落 failed、后续 commit 立即通过。已通过。存量失败用例与未改动的 main 完全一致(均为依赖真实 VLM 端点的环境问题),无回归。本地用的 PyPI 预编译 native 库,建议以 CI 完整结果为准。Follow-up(本 PR 不改)
queue_manager.py的错误路径不 ack、不 tracker.fail,消息会滞留 'processing'。注释显示 no-ack 像是有意设计,建议单独讨论(相关:[bug] reindex 任务 (wait=false) 永久卡在 RUNNING:无 worker 消费、无超时、无 cancel、重启变僵尸 #3396)。🤖 Generated with Claude Code