fix(memory): add trailing newline to JSONL output

[nickveenhof]

Summary

saveGraph writes lines.join("\n") without a trailing newline. The JSONL spec requires every record to be terminated by \n. This makes the output non-conformant and fragile.

The problem

saveGraph produces a file where the last line has no terminating \n:

{"type":"entity","name":"Alice","entityType":"person","observations":["test"]}
{"type":"relation","from":"Alice","to":"Bob","relationType":"knows"}  <-- no \n

A JSONL file should look like this:

{"type":"entity","name":"Alice","entityType":"person","observations":["test"]}\n
{"type":"relation","from":"Alice","to":"Bob","relationType":"knows"}\n

The missing terminator means any tool or process that appends to the file (log rotation, crash recovery, file concatenation) produces invalid JSONL:

{"type":"relation","from":"Alice","to":"Bob","relationType":"knows"}{"type":"entity","name":"External",...}

JSON.parse() then throws on that line:

Unexpected non-whitespace character after JSON at position 88 (line 1 column 89)

How I hit this

I run the memory server across multiple concurrent sessions pointing at the same memory.jsonl file. After several days I found a line with two JSON objects concatenated. Every tool call returned the parse error above until I manually edited the file.

The root cause is likely the race condition described in #2579: multiple MCP server instances calling saveGraph concurrently on the same file. #3286 addresses that with cross-process file locking and atomic writes, which is the proper fix.

This PR does not solve the race condition. It makes the output JSONL-spec-conformant, which is correct regardless of whether #3286 lands.

Fix

// Before
await fs.writeFile(this.memoryFilePath, lines.join("\n"));

// After
await fs.writeFile(this.memoryFilePath, lines.join("\n") + "\n");

loadGraph already filters empty lines (filter(line => line.trim() !== "")), so the trailing \n has no side effects on read.

Tests

Three new tests in knowledge-graph.test.ts:

1. Trailing newline check: verifies the file ends with \n.
2. Per-line validity: verifies every non-empty line parses as valid JSON individually.
3. External append simulation: writes entities via saveGraph, appends a line via fs.appendFile, then reads back via a fresh KnowledgeGraphManager instance. This test fails without the fix and passes with it.

All 48 tests pass. No existing tests affected.

Related

• Bug Report and Resolution: mcp-server-memory Failures due to Race Condition and Environment Misconfiguration #2579: the race condition that causes JSONL corruption with concurrent instances
• Added in-memory locks for the process, fix for Bug #2579 temporary measure. #3060: in-memory lock (closed, insufficient for multi-process)
• feat(memory): add file locking to support multi-instance #3286: cross-process file locking with proper-lockfile (open, the full fix)

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 5s —— View job

---

PR Review: fix(memory): add trailing newline to JSONL output

• Gather context and read changed files
• Review the implementation change
• Review the new tests
• Check for edge cases or issues
• Post final review

---

Summary

This is a clean, well-motivated fix. The change is correct and the JSONL spec compliance argument is solid. A few minor observations below.

---

Implementation (src/memory/index.ts:116)

// Before
await fs.writeFile(this.memoryFilePath, lines.join("\n"));

// After
await fs.writeFile(this.memoryFilePath, lines.join("\n") + "\n");

Correct. This is the right fix. loadGraph already handles trailing newlines gracefully via .filter(line => line.trim() !== "") at line 74, so the read path is unaffected.

Minor edge case — empty graph: When all entities and relations are deleted, lines = [], so lines.join("\n") + "\n" writes "\n" (a single newline) instead of an empty file. Previously it would write "". The load path handles this correctly, but it does mean that after deleting everything, the file contains one blank line rather than nothing. Not a blocking concern, but worth noting.

---

Tests (src/memory/__tests__/knowledge-graph.test.ts)

Three new tests are added to the file persistence describe block.

"should write a trailing newline to produce valid JSONL" (line 429) — Minimal and direct. Exactly right.

"should produce a file where every line is individually valid JSON" (line 438) — The logic is sound and the loop that validates each non-empty line individually is a good defensive check. However, the block comment spanning lines 439–448 is unusually verbose for production test code — it re-explains the bug in prose that belongs in the PR description, not the test. Consider trimming to one line, or removing it entirely since the test name is already self-describing.

"should not corrupt JSONL when content is appended to the file externally" (line 465) — This is the most valuable test: it models the exact failure mode described in the PR. Creating a second KnowledgeGraphManager to force a fresh loadGraph from disk is the right approach. Same feedback applies to the lengthy comment block (lines 468–488).

Pre-existing test at line 412 ("should handle JSONL format correctly") — Note that this test already filters out the trailing empty string produced by splitting on \n, so it would still pass even without this fix. The new trailing-newline test at line 429 is what actually pins the fix in place. This is fine; just clarifying the test coverage picture.

---

Overall

The fix is correct, minimal, and addresses a real spec compliance issue. The three new tests adequately cover the change. The only nit is the long explanatory comment blocks inside the tests — those are better suited to the PR description (where they already exist) than to the source code.

Approve when ready. No blocking issues.
|