mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
fix(coding-agent): keep session memory in line with the file when a write fails (#11486)
When writing a session entry failed (ENOSPC, EBUSY, ETIMEDOUT on network drives), `SessionManager._appendEntry` had already added the entry to memory and made it the current leaf. The process kept running, and the next entry written to disk pointed to a parent that was not in the file. On resume, pi follows the history only back to that gap: everything before it disappears. In #11460 that also dropped the first system message, which declares the tools, so resume turned off every built-in tool. `_appendEntry` now undoes the in-memory append if writing fails, then rethrows. The error reaches the caller as before; later entries chain to the last entry actually on disk. Reports of this problem: - #11460: ENOSPC; resume dropped all built-in tools. - #10166: failure injected with EBUSY; the next entry on disk points to a missing parent. - #10661: ETIMEDOUT on Windows + NFS; resumed sessions showed only the last few messages. That report also describes NUL holes in the file, which this change does not address. **Testing** - Two tests in `test/session-manager/file-operations.test.ts` make writes fail with real file permissions: a read-only session file (failed append) and a read-only session directory (failed first file creation). They check that the entry is not kept, the leaf is unchanged, the next entry chains to the last written one, and the reopened session's branch contains both entries. Both fail without the change and pass with it. - `test/session-manager/` and `test/default-tools-setting.test.ts` pass (140 tests). - `npm run check` passes. Fixes #11460 Requested by @davidbrai Co-authored-by: triagent <triagent@localhost>
This commit is contained in:
parent
269afc66ab
commit
807a5c051f
2 changed files with 50 additions and 1 deletions
|
|
@ -1189,10 +1189,20 @@ export class SessionManager {
|
|||
}
|
||||
|
||||
private _appendEntry(entry: SessionEntry): void {
|
||||
const previousLeafId = this.leafId;
|
||||
this.fileEntries.push(entry);
|
||||
this.byId.set(entry.id, entry);
|
||||
this.leafId = entry.id;
|
||||
this._persist(entry);
|
||||
try {
|
||||
this._persist(entry);
|
||||
} catch (error) {
|
||||
// Keep memory in line with the file: an entry that was not written must not become the
|
||||
// parent of later entries, or the file would hold entries pointing to a missing parent.
|
||||
this.fileEntries.pop();
|
||||
this.byId.delete(entry.id);
|
||||
this.leafId = previousLeafId;
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
/** Append a message as child of current leaf, then advance leaf. Returns entry id.
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import { constants as bufferConstants } from "buffer";
|
||||
import {
|
||||
appendFileSync,
|
||||
chmodSync,
|
||||
closeSync,
|
||||
existsSync,
|
||||
mkdirSync,
|
||||
|
|
@ -471,4 +472,42 @@ describe("SessionManager session file creation", () => {
|
|||
|
||||
expect(readSessionFileRoles(session.getSessionFile()!)).toEqual(["session", "user", "custom", "assistant"]);
|
||||
});
|
||||
|
||||
// #10166, #10661, #11460: a failed write must not leave later entries pointing to a missing parent
|
||||
it("does not keep an entry in memory when writing it fails", () => {
|
||||
const session = SessionManager.create(tempDir, tempDir);
|
||||
const firstId = session.appendMessage(userMsg("first question"));
|
||||
const file = session.getSessionFile()!;
|
||||
|
||||
chmodSync(file, 0o444);
|
||||
expect(() => session.appendMessage(assistantMsg("lost answer"))).toThrow();
|
||||
expect(session.getLeafId()).toBe(firstId);
|
||||
expect(session.getEntries().map((entry) => entry.id)).toEqual([firstId]);
|
||||
|
||||
chmodSync(file, 0o644);
|
||||
const secondId = session.appendMessage(assistantMsg("second answer"));
|
||||
expect(session.getEntry(secondId)?.parentId).toBe(firstId);
|
||||
|
||||
const reopened = SessionManager.open(file, tempDir);
|
||||
expect(reopened.getBranch().map((entry) => entry.id)).toEqual([firstId, secondId]);
|
||||
});
|
||||
|
||||
it("does not keep an entry in memory when creating the file fails", () => {
|
||||
const sessionDir = join(tempDir, "sessions");
|
||||
mkdirSync(sessionDir);
|
||||
const session = SessionManager.create(tempDir, sessionDir);
|
||||
const modelChangeId = session.appendModelChange("anthropic", "claude-sonnet-4-5");
|
||||
|
||||
chmodSync(sessionDir, 0o555);
|
||||
try {
|
||||
expect(() => session.appendMessage(userMsg("first question"))).toThrow();
|
||||
} finally {
|
||||
chmodSync(sessionDir, 0o755);
|
||||
}
|
||||
expect(session.getLeafId()).toBe(modelChangeId);
|
||||
expect(session.getEntries().map((entry) => entry.id)).toEqual([modelChangeId]);
|
||||
|
||||
session.appendMessage(userMsg("second question"));
|
||||
expect(readSessionFileRoles(session.getSessionFile()!)).toEqual(["session", "model_change", "user"]);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue