Skip to content

fix(channels): await persisted session hydration before enabling plugins#1888

Merged
piorpua merged 2 commits intoiOfficeAI:mainfrom
amanharshx:fix/session-hydration-race
Mar 30, 2026
Merged

fix(channels): await persisted session hydration before enabling plugins#1888
piorpua merged 2 commits intoiOfficeAI:mainfrom
amanharshx:fix/session-hydration-race

Conversation

@amanharshx
Copy link
Copy Markdown
Contributor

@amanharshx amanharshx commented Mar 29, 2026 •
edited
Loading

Closes #1889

Summary

  • Expose loadActiveSessions() promise as a public ready property on SessionManager
  • Await sessionManager.ready in ChannelManager.initialize() before creating PluginManager and loading plugins

Motivation

SessionManager fires async hydration (loadActiveSessions()) in its constructor without awaiting it.
ChannelManager.initialize() then immediately passes the session manager to plugins. Today this works
by accident — both hydration and plugin loading share the same getDatabase() promise, so microtask
ordering guarantees hydration completes first. But this is implicit and fragile: any future refactor
(caching the DB instance, reordering startup) could silently break it, causing getSession() to miss
persisted sessions and create duplicate conversations.

Diff

+5 −1 across 2 files (no logic changes, explicit await added)

Files changed

  • src/process/channels/core/SessionManager.ts — store hydration promise as ready property
  • src/process/channels/core/ChannelManager.ts — await this.sessionManager.ready before plugin setup

Testing

  • bunx tsc --noEmit — no type errors
  • bun run test — 206 files passed, 2023 tests passed
  • Manual: startup order unchanged, sessions load before plugins start

Risks / Side effects

  • Minimal — adds an explicit await for something that was already completing before plugin start
  • No behavioral change in the current code path; purely defensive hardening

@amanharshx amanharshx mentioned this pull request Mar 29, 2026
Closed
@sentry
Copy link
Copy Markdown

sentry bot commented Mar 29, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/process/channels/core/ChannelManager.ts 0.00% 1 Missing ⚠️
src/process/channels/core/SessionManager.ts 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@piorpua piorpua added the bot:reviewing Review in progress (mutex) label Mar 30, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 30, 2026

CI 检查未通过

以下 job 在本次自动化 review 时未通过，请修复：

Job 结论
Unit Tests (windows-2022) ❌ CANCELLED
Build Test (macos-arm64) ❌ CANCELLED
Build Test (macos-x64) ❌ CANCELLED
Build Test (windows-x64) ❌ CANCELLED
Build Test (windows-arm64) ❌ CANCELLED
Build Test (linux) ❌ CANCELLED

本次自动化 review 暂缓，待 CI 全部通过后将重新处理。

@piorpua piorpua added bot:ci-waiting CI failed and author notified — snoozed until new commits are pushed and removed bot:reviewing Review in progress (mutex) labels Mar 30, 2026
@piorpua piorpua added bot:reviewing Review in progress (mutex) and removed bot:ci-waiting CI failed and author notified — snoozed until new commits are pushed labels Mar 30, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 30, 2026

Code Review：fix(channels): await persisted session hydration before enabling plugins (#1888)

变更概述

本 PR 修复了 SessionManager 初始化时的隐式竞态条件：原先构造函数中的 loadActiveSessions() 异步调用没有被 await，仅靠 getDatabase() 的 microtask 顺序巧合保证了在插件启动之前完成。本 PR 将 hydration Promise 暴露为 ready 属性，并在 ChannelManager.initialize() 中显式 await，改动共 +5 / -1 行，涉及 SessionManager.ts 和 ChannelManager.ts。

方案评估

结论：✅ 方案合理

将异步初始化结果存储为 readonly ready: Promise<void> 是 TypeScript 类中处理"构造器无法 await"问题的标准惯用模式。方案最小化且精准，避免了引入工厂方法或额外 initialize() 层的复杂度。ready 会在 initialize() 的外层 try-catch 下传播错误，错误处理链完整。与现有架构边界一致，无过度工程化。

问题清单

🔵 LOW — 新增代码缺乏测试覆盖

文件：src/process/channels/core/SessionManager.ts:26、src/process/channels/core/ChannelManager.ts:85

问题说明：Codecov patch 覆盖率为 0%，两条新增行均无测试覆盖。ready 属性的行为（包括 hydration 失败时的错误传播、成功时 activeSessions 已填充）缺乏专项测试。虽然 codecov.yml 将此 check 设为 informational: true 不阻塞合并，但建议补充覆盖。

修复建议：在 SessionManager 的测试文件中添加：

it('should expose ready promise that resolves after session hydration', async () => {
  const manager = new SessionManager();
  await expect(manager.ready).resolves.toBeUndefined();
  // verify activeSessions populated from mocked DB
});

汇总

# 严重级别 文件 问题
1 🔵 LOW SessionManager.ts:26, ChannelManager.ts:85 新增代码 patch 覆盖率 0%

结论

✅ 批准合并 — 无阻塞性问题

方案正确且最小化，解决了真实的隐式竞态条件。唯一问题为低优先级的测试覆盖缺失，不影响合并。

本报告由本地 pr-review skill 生成，包含完整项目上下文，无截断限制。

@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 30, 2026

✅ 已自动 review，无阻塞性问题，正在触发自动合并。

@piorpua piorpua merged commit 9249a5d into iOfficeAI:main Mar 30, 2026
13 of 14 checks passed
@piorpua piorpua added bot:done Auto-merged by bot and removed bot:reviewing Review in progress (mutex) labels Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bot:done Auto-merged by bot

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Session restoration has implicit startup race in SessionManager hydration

2 participants

@amanharshx @piorpua