fix(fs): 崩溃残留半行截断，杜绝事件流永久不可读（P0） - #29
Open
echoVic wants to merge 1 commit into
Open
Conversation
appendLineSync 用 O_APPEND 打开后直接写入,从不检查既有文件末字节。崩溃残留 的无换行半行会被下一次追加粘成中间损坏行,再追加一条后 readJsonlTolerant 按 「非末行损坏」抛错,整个 events.jsonl 永久不可读。 改用 'a+' 打开,追加前 fstat + 读末字节,非 '\n' 即判定为崩溃残留半行并 ftruncate 截断到最后一个完整行之后(反向分块扫描定位换行),再写新记录。因每条 已提交记录都以 '\n' 结尾,「不以换行收尾」唯一标识 readJsonlTolerant 本就视作 未写入的半行,物理丢弃安全。 补充崩溃尾态回归单测:截断而非粘连、空文件、全文件无换行、多字节 UTF-8 中途 截断、完整 JSON 缺换行,以及崩溃后连追两条整表仍可读的缺陷复现。
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
Repair incomplete JSONL tails packages/boss-cli/src/infrastructure/fs.ts |
appendLineSync opens the file for read/write append access, truncates incomplete trailing data, appends the new record, and fsyncs the descriptor. Helpers scan backward to locate the last newline. |
Validate crash recovery test/infrastructure/append-after-crash.defect.test.ts, test/infrastructure/jsonl-atomicity.test.ts |
Tests cover partial files, UTF-8 truncation, missing final newlines, fresh files, and repeated appends that preserve readable records. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Possibly related PRs
- echoVic/boss-skill#28: Adds the earlier
appendLineSyncJSONL atomicity behavior extended by this recovery change.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly identifies the crash-residue truncation fix and its purpose of preserving event-stream readability. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix-append-crash-residue
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/infrastructure/append-after-crash.defect.test.ts`:
- Around line 24-45: Update the documentation comment for the appendLineSync
regression tests to remove claims that the implementation currently fails or
leaves appended events corrupt. Describe that the tests verify recovery from an
unterminated trailing line and that appendLineSync repairs the missing newline
before appending, with the tests expected to pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4470d13a-1fca-4881-a666-55dc8616050f
📒 Files selected for processing (3)
packages/boss-cli/src/infrastructure/fs.tstest/infrastructure/append-after-crash.defect.test.tstest/infrastructure/jsonl-atomicity.test.ts
Comment on lines
+24
to
+45
| /** | ||
| * DEFECT PROOF — appendLineSync does not repair a missing trailing newline. | ||
| * | ||
| * The documented atomicity contract (infrastructure/fs.ts) states that a | ||
| * process killed mid-append leaves "at most one incomplete trailing line" | ||
| * which is "recoverable (treated as if the event was never written)". The | ||
| * recovery path (state.ts#appendEvent) derives the next id from the parseable | ||
| * records and appends the new event with appendLineSync. | ||
| * | ||
| * A real crash mid-append leaves the file WITHOUT a trailing newline. Because | ||
| * appendLineSync only ensures the line it writes ends with '\n' (it never | ||
| * checks whether the existing file content is newline-terminated), the new | ||
| * event is glued onto the truncated tail, producing a single corrupt line. | ||
| * The freshly-written event is then unreadable, and once one more event is | ||
| * appended the corrupt line is no longer the tail — readJsonlTolerant THROWS, | ||
| * making the entire events.jsonl (the event-sourcing source of truth) | ||
| * permanently unreadable. This is the exact failure mode the design claims to | ||
| * prevent. | ||
| * | ||
| * These tests are EXPECTED TO FAIL against the current implementation; they | ||
| * pin the correct behavior a fix must satisfy (appendLineSync should ensure | ||
| * the previous content is newline-terminated before appending). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the obsolete defect-status comment.
appendLineSync now repairs an unterminated tail. Lines 25 and 33-45 describe the old behavior as current and state that these tests should fail. Update the comment to describe regression coverage and the expected passing behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/infrastructure/append-after-crash.defect.test.ts` around lines 24 - 45,
Update the documentation comment for the appendLineSync regression tests to
remove claims that the implementation currently fails or leaves appended events
corrupt. Describe that the tests verify recovery from an unterminated trailing
line and that appendLineSync repairs the missing newline before appending, with
the tests expected to pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
缺陷（P0，ROBUSTNESS，两检测器独立复现，置信度 9）
appendLineSync（事件溯源真相源events.jsonl的唯一追加入口）用O_APPEND打开后直接写入，从不检查既有文件末字节是否为换行。失效链：
{"id":1}\n{"id":2}\n{"id":3）。state.ts#appendEvent用readJsonlTolerant跳过损坏尾行推导 id 后再appendLineSync→ 新记录被粘到半行上形成一条中间损坏行。readJsonlTolerant按「非末行损坏」抛错 → 整个 events.jsonl 永久不可读。这与该基础设施 docstring / PRIVACY / CHANGELOG 明确承诺的「崩溃最多留一条可恢复末行、视作该事件未写入」直接矛盾。
修复
appendLineSync改用'a+'（O_APPEND | O_RDWR）打开；追加前fstat取大小并读末字节，非\n即判定为崩溃残留半行，用ftruncate截断到最后一个完整行之后（反向分块扫描定位换行，避免整文件读入）再写新记录。安全性论证：本函数保证每条已提交记录都以
\n结尾，因此「文件不以换行收尾」唯一标识一条readJsonlTolerant本就视作「从未写入」的崩溃半行 —— 物理丢弃它不会损失任何已提交事件。测试
新增崩溃尾态回归覆盖：截断而非粘连、空文件不受影响、全文件无换行、多字节 UTF-8 中途截断、完整 JSON 缺换行（未 fsync 残留），以及崩溃后连追两条整表仍可读的缺陷复现。
npm run build/npm run typecheck通过已知后续项（不在本 PR 范围）
审查另提出一条 P2 并发副作用（多写入者并发 append 时
fstat→ftruncate非原子，可能误截并发写入者数据）。boss 事件流实际为单写入者，故留待后续处理，本 PR 不含该改动。Summary by CodeRabbit
Bug Fixes
Tests