Merge PR #8491: fix(agent): remove capped empty stop tails (@roboomp)
# Conflicts: # packages/coding-agent/src/session/agent-session.ts # packages/coding-agent/src/session/session-manager.ts # packages/coding-agent/test/agent-session-empty-stop-guard.test.ts # packages/coding-agent/test/session-manager-immediate-persist.test.ts
This commit is contained in:
@@ -1797,6 +1797,9 @@
|
||||
- Fixed agents getting stuck waiting for messages from peers that have already stopped running.
|
||||
- Fixed compiled Linux binary extension loading when bundled web-search header generation cannot read `header-generator` data files from the build-time path. ([#5178](https://github.com/can1357/oh-my-pi/issues/5178))
|
||||
- Fixed plugin custom tool loading to skip and report invalid feature entries instead of crashing startup when a plugin dependency tree leaves one feature unresolved. ([#5189](https://github.com/can1357/oh-my-pi/issues/5189))
|
||||
### Fixed
|
||||
|
||||
- Fixed empty local-model stops lingering on the persisted active branch after retries; discarded turns now durably select their parent, preserve safe metadata children, and cannot resurface after reload or a mid-retry process kill. ([#5179](https://github.com/can1357/oh-my-pi/issues/5179))
|
||||
|
||||
## [16.4.4] - 2026-07-11
|
||||
|
||||
|
||||
@@ -7094,6 +7094,7 @@ export class AgentSession {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
|
||||
#enforceRewindBeforeYield(): boolean {
|
||||
if (!this.#checkpointState || this.#pendingRewindReport) {
|
||||
return false;
|
||||
|
||||
@@ -247,7 +247,7 @@ export interface SessionMaintenanceHost {
|
||||
dropImages(): Promise<{ removed: number }>;
|
||||
runHandoff(customInstructions?: string, options?: SessionHandoffOptions): Promise<HandoffResult | undefined>;
|
||||
removeAssistantMessageFromActiveContext(message: AssistantMessage): void;
|
||||
dropPersistedAssistantTurn(message: AssistantMessage): Promise<void>;
|
||||
dropPersistedAssistantTurn(message: AssistantMessage): Promise<string | undefined>;
|
||||
runRecoveryCompactionWithRollback(
|
||||
reason: "overflow" | "incomplete",
|
||||
message: AssistantMessage,
|
||||
|
||||
@@ -90,6 +90,7 @@ import {
|
||||
|
||||
const JSONL_SUFFIX_LENGTH = ".jsonl".length;
|
||||
const DRAFT_ONLY_SESSION_MARKER = ".draft-only-session";
|
||||
const DISCARDED_ENTRY_BRANCH_MARKER = "discarded-entry-branch";
|
||||
|
||||
function mintSessionId(): string {
|
||||
return Bun.randomUUIDv7();
|
||||
@@ -2465,6 +2466,35 @@ export class SessionManager {
|
||||
this.#setLeaf(null);
|
||||
}
|
||||
|
||||
/**
|
||||
* Durably move the active branch past a discarded entry.
|
||||
*
|
||||
* The loader reconstructs the active branch from the last physical journal
|
||||
* entry, so changing the in-memory leaf alone is lost on reload. Known
|
||||
* metadata children are chained onto the discarded entry's parent before the
|
||||
* entry is removed. If any child may carry content, the subtree is preserved
|
||||
* off-branch instead. Both paths append a metadata-only branch marker and
|
||||
* rewrite the journal, making the selected path durable.
|
||||
*/
|
||||
async discardEntryDurably(entryId: string): Promise<void> {
|
||||
const entry = this.#index.get(entryId);
|
||||
if (!entry) return;
|
||||
const children = this.#index.childrenOf(entryId);
|
||||
const canReparentChildren = children.every(child => child.type === "service_tier_change");
|
||||
let leafId = entry.parentId;
|
||||
if (canReparentChildren) {
|
||||
for (const child of children) {
|
||||
child.parentId = leafId;
|
||||
leafId = child.id;
|
||||
}
|
||||
this.#entries = this.#entries.filter(candidate => candidate.id !== entryId);
|
||||
this.#index.rebuild(this.#entries);
|
||||
}
|
||||
this.#index.setLeaf(leafId);
|
||||
this.appendCustomEntry(DISCARDED_ENTRY_BRANCH_MARKER, { discardedEntryId: entryId });
|
||||
await this.rewriteEntries();
|
||||
}
|
||||
|
||||
/** Like branch(), but also records a branch_summary of the abandoned path. */
|
||||
branchWithSummary(branchFromId: string | null, summary: string, details?: unknown, fromExtension?: boolean): string {
|
||||
if (branchFromId !== null && !this.#index.has(branchFromId)) throw new Error(`Entry ${branchFromId} not found`);
|
||||
|
||||
@@ -409,8 +409,8 @@ export class TurnRecovery {
|
||||
return this.#handleUnexpectedAssistantStop(message);
|
||||
}
|
||||
|
||||
/** Removes a persisted failed assistant turn after its persistence slot settles. */
|
||||
dropPersistedAssistantTurn(message: AssistantMessage): Promise<void> {
|
||||
/** Removes a persisted failed assistant turn after its persistence slot settles; returns the dropped branch entry id. */
|
||||
dropPersistedAssistantTurn(message: AssistantMessage): Promise<string | undefined> {
|
||||
return this.#dropPersistedAssistantTurn(message);
|
||||
}
|
||||
|
||||
@@ -720,10 +720,14 @@ export class TurnRecovery {
|
||||
// provider usage can anchor the next prompt at the full failed-request size
|
||||
// and re-trigger compaction at the same boundary. Remove every capped
|
||||
// empty output; toolUse orphans still need this for Anthropic history.
|
||||
await this.dropPersistedAssistantTurn(assistantMessage);
|
||||
await this.#dropAssistantTurnDurably(assistantMessage);
|
||||
return "terminal";
|
||||
}
|
||||
this.discardAssistantTurn(assistantMessage);
|
||||
// The reparented leaf must be durably persisted before the retry continues:
|
||||
// the loader rebuilds the active branch from the last physical entry, so an
|
||||
// in-memory-only reparent lets the empty stop resurface on reload or after
|
||||
// a mid-retry process kill.
|
||||
await this.#dropAssistantTurnDurably(assistantMessage);
|
||||
this.#host.agent.appendMessage({
|
||||
role: "developer",
|
||||
content: [{ type: "text", text: this.#emptyStopRetryReminder() }],
|
||||
@@ -846,9 +850,19 @@ export class TurnRecovery {
|
||||
* replays the failed turn, while no-recovery paths leave the persisted entry
|
||||
* (and the user-visible transcript line) in place.
|
||||
*/
|
||||
async #dropPersistedAssistantTurn(assistantMessage: AssistantMessage): Promise<void> {
|
||||
async #dropPersistedAssistantTurn(assistantMessage: AssistantMessage): Promise<string | undefined> {
|
||||
await this.#host.waitForSessionMessagePersistence(assistantMessage);
|
||||
this.discardAssistantTurn(assistantMessage);
|
||||
return this.discardAssistantTurn(assistantMessage);
|
||||
}
|
||||
|
||||
/**
|
||||
* Drop the failed turn from active context and the persisted branch, then
|
||||
* durably persist the reparented leaf so the empty stop cannot resurface on
|
||||
* reload or after a mid-retry process kill.
|
||||
*/
|
||||
async #dropAssistantTurnDurably(assistantMessage: AssistantMessage): Promise<void> {
|
||||
const droppedEntryId = await this.#dropPersistedAssistantTurn(assistantMessage);
|
||||
if (droppedEntryId) await this.#host.sessionManager.discardEntryDurably(droppedEntryId);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -940,7 +954,7 @@ export class TurnRecovery {
|
||||
* the Gemini header-runaway interrupt, which must not replay a partial,
|
||||
* loop-fueling thinking block.
|
||||
*/
|
||||
discardAssistantTurn(assistantMessage: AssistantMessage): void {
|
||||
discardAssistantTurn(assistantMessage: AssistantMessage): string | undefined {
|
||||
this.removeAssistantMessageFromActiveContext(assistantMessage);
|
||||
|
||||
const branch = this.#host.sessionManager.getBranch();
|
||||
@@ -962,7 +976,7 @@ export class TurnRecovery {
|
||||
this.#isSameAssistantMessage(entry.message as AssistantMessage, assistantMessage),
|
||||
);
|
||||
if (!branchEntry) {
|
||||
return;
|
||||
return undefined;
|
||||
}
|
||||
this.#host.withBashBranchTransition(() => {
|
||||
if (branchEntry.parentId === null) {
|
||||
@@ -971,6 +985,7 @@ export class TurnRecovery {
|
||||
this.#host.sessionManager.branch(branchEntry.parentId);
|
||||
}
|
||||
});
|
||||
return branchEntry.id;
|
||||
}
|
||||
|
||||
#isSameAssistantMessage(left: AssistantMessage, right: AssistantMessage): boolean {
|
||||
|
||||
@@ -231,6 +231,10 @@ describe("AgentSession empty stop guard", () => {
|
||||
.filter(entry => entry.type === "message")
|
||||
.map(entry => entry.message as AgentMessage);
|
||||
expect(emptyAssistantStops(activeBranchMessages)).toHaveLength(0);
|
||||
// A discarded empty stop is physically removed from the journal, not just
|
||||
// reparented off the active branch: it must never be able to resurface as
|
||||
// the active leaf on reload (the loader rebuilds from the last physical
|
||||
// entry) if the process is killed before the recovery turn lands.
|
||||
expect(
|
||||
emptyAssistantStops(
|
||||
session.sessionManager
|
||||
@@ -238,7 +242,7 @@ describe("AgentSession empty stop guard", () => {
|
||||
.filter(entry => entry.type === "message")
|
||||
.map(entry => entry.message as AgentMessage),
|
||||
),
|
||||
).toHaveLength(1);
|
||||
).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("retries a tool-use stop that has no tool call or text", async () => {
|
||||
@@ -392,6 +396,17 @@ describe("AgentSession empty stop guard", () => {
|
||||
.filter(entry => entry.type === "message")
|
||||
.map(entry => entry.message as AgentMessage);
|
||||
expect(emptyAssistantStops(activeBranchMessages)).toHaveLength(0);
|
||||
|
||||
// The loader reconstructs the active branch from the last physical journal
|
||||
// entry. The empty stop is removed from history and a marker durably
|
||||
// selects its parent, so reload cannot reactivate the discarded turn.
|
||||
const journalMessages = session.sessionManager
|
||||
.getEntries()
|
||||
.filter(entry => entry.type === "message")
|
||||
.map(entry => entry.message as AgentMessage);
|
||||
expect(emptyAssistantStops(journalMessages)).toHaveLength(0);
|
||||
const lastJournalEntry = session.sessionManager.getEntries().at(-1);
|
||||
expect(lastJournalEntry).toMatchObject({ type: "custom", customType: "discarded-entry-branch" });
|
||||
});
|
||||
|
||||
it("waits for capped empty-stop persistence before removing the active branch entry", async () => {
|
||||
|
||||
@@ -365,4 +365,59 @@ describe("SessionManager JSONL software-crash durability", () => {
|
||||
writeSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("reparents metadata children when durably discarding an entry", async () => {
|
||||
const cwd = makeTempDir("@pi-discard-metadata-cwd-");
|
||||
const sessionDir = path.join(cwd, "sessions");
|
||||
const manager = SessionManager.create(cwd, sessionDir);
|
||||
const sessionFile = manager.getSessionFile();
|
||||
if (!sessionFile) throw new Error("Expected a persisted session file path");
|
||||
|
||||
const priorId = manager.appendMessage(assistantMessage("prior turn"));
|
||||
const discardedId = manager.appendMessage(assistantMessage(""));
|
||||
const serviceTierId = manager.appendServiceTierChange(null);
|
||||
await manager.discardEntryDurably(discardedId);
|
||||
await manager.close();
|
||||
|
||||
const reloaded = await SessionManager.open(sessionFile, sessionDir);
|
||||
const branch = reloaded.getBranch();
|
||||
expect(branch.some(entry => entry.id === discardedId)).toBe(false);
|
||||
expect(branch).toContainEqual(expect.objectContaining({ id: serviceTierId, parentId: priorId }));
|
||||
expect(branch.at(-1)).toMatchObject({
|
||||
type: "custom",
|
||||
customType: "discarded-entry-branch",
|
||||
parentId: serviceTierId,
|
||||
});
|
||||
await reloaded.close();
|
||||
});
|
||||
|
||||
it("persists a branch marker when a discarded entry has content children", async () => {
|
||||
const cwd = makeTempDir("@pi-discard-content-cwd-");
|
||||
const sessionDir = path.join(cwd, "sessions");
|
||||
const manager = SessionManager.create(cwd, sessionDir);
|
||||
const sessionFile = manager.getSessionFile();
|
||||
if (!sessionFile) throw new Error("Expected a persisted session file path");
|
||||
|
||||
const priorId = manager.appendMessage(assistantMessage("prior turn"));
|
||||
const discardedId = manager.appendMessage(assistantMessage(""));
|
||||
const contentChildId = manager.appendMessage({
|
||||
role: "user",
|
||||
content: "preserve off branch",
|
||||
timestamp: Date.now(),
|
||||
});
|
||||
await manager.discardEntryDurably(discardedId);
|
||||
await manager.close();
|
||||
|
||||
const reloaded = await SessionManager.open(sessionFile, sessionDir);
|
||||
const branch = reloaded.getBranch();
|
||||
expect(reloaded.getEntries()).toContainEqual(expect.objectContaining({ id: discardedId }));
|
||||
expect(reloaded.getEntries()).toContainEqual(expect.objectContaining({ id: contentChildId }));
|
||||
expect(branch.some(entry => entry.id === discardedId || entry.id === contentChildId)).toBe(false);
|
||||
expect(branch.at(-1)).toMatchObject({
|
||||
type: "custom",
|
||||
customType: "discarded-entry-branch",
|
||||
parentId: priorId,
|
||||
});
|
||||
await reloaded.close();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user