Skip to content

fix(sessions): settle native dedupe ties deterministically - #1385

Merged
vastsa merged 3 commits into
vastsa:mainfrom
shabhui:fix/session-dedup-deterministic-tiebreak
Oct 4, 2026
Merged

vastsa merged 3 commits into
vastsa:mainfrom
shabhui:fix/session-dedup-deterministic-tiebreak

Conversation

@shabhui

@shabhui shabhui commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Problem

#1383: NativePiSessionService.list() dedupes copied session files by keeping the newest write per native id, but updatedAt is derived from file content. A byte-identical copy always ties with the original, so "keep the newest write" never resolves the exact case the dedup (#1359 / #1374) was added for. The tie fell through to walkJsonl order, which follows directory enumeration and differs per platform:

  • NTFS (alphabetical readdir + LIFO stack): the backup/ copy is walked first and wins, the original's record is deleted from records, and the new regression test from fix(sessions): specify native duplicate identity handling #1374 fails on Windows.
  • ext4 (hash order): the original happens to win, so CI stayed green and masked the difference.

Consequences on Windows: a kept .jsonl backup takes the session over — reopening from the sidebar appends to the copy while the original freezes, persisted references to the original id get NOT_FOUND, and a live runtime's record can be deleted mid-session.

Fix

Order dedupe ties deterministically instead of trusting enumeration order:

  1. an id with an open runtime keeps the file that runtime is already writing;
  2. the last tie is settled by path order, which every platform computes the same way.

Tests

  • breaks copy ties by path order instead of directory enumeration (#1383) — a copy in a directory sorting before the original's group still yields exactly one entry, and the winner is deterministic across platforms.
  • keeps the open runtime's file when a copy ties on recency (#1383) — after a faux-model turn, a byte-identical copy sorting before the original loses to the open runtime.
  • The existing #1359 test (copy in backup/, original wins) now passes deterministically on all platforms; it failed on NTFS before this change.

Local validation (Windows 11 / NTFS):

  • vitest run src/native-pi-session.test.ts — 39/39 pass (37 before + 2 new)
  • tsc -b packages/agent-runtime — clean
  • biome: packages/agent-runtime is outside the repo's biome.json include list (checked, no findings possible)

fixes #1383

Byte-identical copies share the content-derived updatedAt, so the
"keep the newest write" order in list() tied for exactly the manual-copy
case the dedup was added for. The tie fell through to walkJsonl order,
which follows directory enumeration and flips per platform: on NTFS a
backup/ copy won while ext4 kept the original, so the copy took the
session over, the original's id was dropped from records, and the new
regression test failed on Windows.

Break ties deterministically: an id with an open runtime keeps the file
it is already writing, and the last tie is settled by path order, which
every platform computes the same way.

fixes vastsa#1383
vastsa added 2 commits October 4, 2026 17:16
The vastsa#1359 regression test only scanned its "control" after the copy
existed, so it compared the winner with itself: it passed on every
platform even while the byte-identical copy won the dedup on NTFS and
APFS. Scan the control before writing the copy so the assertion pins
which file stays canonical for the native id, and let it fail if the
winner ever follows enumeration order again.

Also lift the inline faux model runtime that two tests duplicated into
one `fauxModelRuntime()` helper and document the rankTie comparator,
including the id fallback that only keeps it total.

Verified locally: the strengthened assertion fails on main (the copy
wins on APFS) and passes with the deterministic tie-break; the package
suite is 1240/1240 and `tsc --noEmit` is clean.
@vastsa
vastsa merged commit 675c4aa into vastsa:main Oct 4, 2026
4 checks passed
@vastsa

vastsa commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Maintainer follow-up (a2eb0a5, included in this PR)

  1. The [Bug] 会话列表出现重复会话:递归扫描 + 按路径生成 ID,子目录副本变成第二条会话 #1359 assertion was a no-op. That test built its "control" scan after the copy existed, so it compared the winner with itself and passed on every platform — including the NTFS/APFS enumeration order where the copy wins. Reproduced on APFS against the original code: the copy takes the id over, and the test still passed. So the "it failed on NTFS before this change" claim does not hold. The control now scans before the copy is written, which fails on main (copy wins) and passes with this fix.
  2. Extracted the inline faux model runtime that two tests duplicated into fauxModelRuntime().
  3. Documented the rankTie comparator (why it is a total order, and what the id fallback is for).

The tie-break logic itself is unchanged (open runtime → smallest record path) and was re-verified as order-independent under forced reversed readdir. Suite: 1240/1240, tsc --noEmit clean.

@shabhui

shabhui commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

感谢审阅、强化与合并!

补一个事后核对,两处更正:

  1. 我在 [Bug] 会话去重(#1374)平局裁决依赖文件系统遍历顺序:Windows 上副本反向胜出,新回归测试在 NTFS 必挂 #1383 里说原 [Bug] 会话列表出现重复会话:递归扫描 + 按路径生成 ID,子目录副本变成第二条会话 #1359 测试"在 NTFS 上必挂",这个说法不成立。 原测试的 control(list() 扫描)发生在写入副本之后,胜者是在和自身比较,因此在任何平台上都能通过——它其实是个 no-op。您把 control 移到写入副本之前并抽出 fauxModelRuntime(),才让它真正具备了捕获能力(16950ee)。

  2. 为了确认这一点,我在 Windows/NTFS 上把强化后的测试放回合并前的代码(831b66d)跑了一遍:2 failed | 37 passed——失败的正是强化后的 [Bug] 会话列表出现重复会话:递归扫描 + 按路径生成 ID,子目录副本变成第二条会话 #1359 用例和新增的 [Bug] 会话去重(#1374)平局裁决依赖文件系统遍历顺序:Windows 上副本反向胜出,新回归测试在 NTFS 必挂 #1383 路径序用例,与您在 APFS 上的结果一致。也再次印证 rankTie 的路径字典序裁决在 NTFS 上行为相同(字节级副本平局 → 副本胜)。

感谢修正,学到了:control 必须发生在被测变更之前,否则断言恒真。

@shabhui
shabhui deleted the fix/session-dedup-deterministic-tiebreak branch October 4, 2026 09:50

This branch was previously deployed

1 inactive deployment
Preview — a2eb0a57 Deployed Oct 4, 2026 by vercel[bot]
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.

[Bug] 会话去重(#1374)平局裁决依赖文件系统遍历顺序:Windows 上副本反向胜出,新回归测试在 NTFS 必挂

2 participants