fix(pi): contain historian failures and busy-starved lease releases

[randomvariable]

The Pi historian run promise is parked in inFlightHistorian with no catch, so a SQLITE_BUSY from the finally-block lease release escaped as an unhandled rejection via processTicksAndRejections. OpenCode's startCompartmentAgent carries its own .catch; the Pi port dropped it.

• spawnPiHistorianRun: add .catch (session log) and guard both lease releases as best-effort teardown; the row expires via its 5-min TTL.
• runPiWrapup: catch historian chunk failures and surface them as a Partial result instead of letting them escape the command handler, and guard the per-chunk lease release the same way.
• Add a regression test that holds BEGIN IMMEDIATE on a second connection (shared file, not :memory:) and asserts wrapup survives the busy-starved release.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Previously, Pi historian failures and busy-starved lease releases could escape as unhandled rejections and abort /ctx-wrapup. Now historian errors are logged, wrapup returns a Partial result, and failed lease deletes expire through their five-minute TTL instead.

Bug Fixes

• Failed historian chunks report the last completed message so a later /ctx-wrapup can continue.
• Adds a file-backed SQLite lock-contention regression test for lease release.

^{Written for commit 710b343. Summary will update on new commits.}

Greptile Summary

The PR contains Pi historian failures so lease-release contention cannot escape as an unhandled rejection, and converts wrapup historian exceptions into partial results.

• Guards background and wrapup lease releases as best-effort teardown.
• Logs otherwise escaping background historian failures before in-flight cleanup.
• Adds a file-backed SQLite contention regression test for wrapup.

Confidence Score: 4/5

The PR appears safe to merge, with non-blocking cleanup and error-sentinel issues worth correcting.

Failure containment remains intact, but an empty exception message can bypass the intended partial-failure branch, and the new regression test leaks its file-backed SQLite database.

Files Needing Attention: packages/pi-plugin/src/commands/ctx-wrapup.ts; packages/pi-plugin/src/commands/ctx-wrapup.test.ts

Important Files Changed

Filename	Overview
packages/pi-plugin/src/commands/ctx-wrapup.ts	Converts historian exceptions into partial wrapup results and guards lease teardown, but its truthiness-based failure sentinel mishandles empty error messages.
packages/pi-plugin/src/context-handler.ts	Contains background historian and lease-release rejections while retaining in-flight cleanup and status notification.
packages/pi-plugin/src/commands/ctx-wrapup.test.ts	Adds realistic cross-connection SQLite contention coverage but leaves the generated temporary database file behind.

_{Reviews (1): Last reviewed commit: "fix(pi): contain historian failures and ..." | Re-trigger Greptile}

Greptile also left 2 inline comments on this PR.

Context used:

• Knowledge Base — Pi plugin runtime

[greptile-apps]

Empty failures bypass containment

When the historian throws an error with an empty message, chunkFailure becomes an empty string and this truthiness check treats it as no exception. Wrapup then continues or reports no forward progress instead of stopping with the intended historian-failure result.

Suggested change

	if (chunkFailure) {
	if (chunkFailure !== null) {

[greptile-apps]

Temporary database files accumulate

Each execution creates a UUID-named SQLite file under the system temporary directory, but teardown only closes the database handle. Repeated local and CI runs therefore leave the generated pi-wrapup-busy-release-*.db files behind.

[cubic-dev-ai]

5 issues found across 3 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/pi-plugin/src/commands/ctx-wrapup.test.ts">

<violation number="1" location="packages/pi-plugin/src/commands/ctx-wrapup.test.ts:45">
P3: Each execution leaves the file-backed test database in the shared temporary directory. Track the generated path and remove the database artifacts in `finally`, preferably by creating a temporary directory and deleting that directory after closing all connections.</violation>

<violation number="2" location="packages/pi-plugin/src/commands/ctx-wrapup.test.ts:333">
P3: Delete the temporary SQLite file after closing the database; this test creates a UUID-named file under `tmpdir()`, but `closeQuietly(db)` only closes the handle, so repeated runs leave `pi-wrapup-busy-release-*.db` files behind.</violation>
</file>

<file name="packages/pi-plugin/src/context-handler.ts">

<violation number="1" location="packages/pi-plugin/src/context-handler.ts:3756">
P2: When `historian.onStatusChange` throws during teardown, this `.catch` has already completed, so the promise returned by `.finally` rejects without a handler and can still trigger an unhandled rejection. Wrap the finalizer in a `try/catch` or add a catch after `.finally` so cleanup and status-update failures are contained too.</violation>
</file>

<file name="packages/pi-plugin/src/commands/ctx-wrapup.ts">

<violation number="1" location="packages/pi-plugin/src/commands/ctx-wrapup.ts:473">
P2: When a historian rejects with an empty error message, `chunkFailure` is `""`, so this check treats the failed chunk as successful progress. Track failure with a boolean or an object instead of using the formatted message as the sentinel.</violation>

<violation number="2" location="packages/pi-plugin/src/commands/ctx-wrapup.ts:474">
P2: When a historian publishes the chunk and then `onPublished` throws, `lastEnd` still contains the pre-chunk value, so the Partial result reports the wrong wrapped-through message. Refresh `lastEnd` from `getLastCompartmentEndMessage` before formatting this failure.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: When historian.onStatusChange throws during teardown, this .catch has already completed, so the promise returned by .finally rejects without a handler and can still trigger an unhandled rejection. Wrap the finalizer in a try/catch or add a catch after .finally so cleanup and status-update failures are contained too.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/context-handler.ts, line 3756:

<comment>When `historian.onStatusChange` throws during teardown, this `.catch` has already completed, so the promise returned by `.finally` rejects without a handler and can still trigger an unhandled rejection. Wrap the finalizer in a `try/catch` or add a catch after `.finally` so cleanup and status-update failures are contained too.</comment>

<file context>
@@ -3726,15 +3734,32 @@ function spawnPiHistorianRun(args: {
+			// processTicksAndRejections instead of surfacing as a session log.
+			sessionLog(sessionId, "historian run failed:", err);
+		})
+		.finally(() => {
+			inFlightHistorian.delete(sessionId);
+			unregister();
</file context>

[cubic-dev-ai]

P2: When a historian publishes the chunk and then onPublished throws, lastEnd still contains the pre-chunk value, so the Partial result reports the wrong wrapped-through message. Refresh lastEnd from getLastCompartmentEndMessage before formatting this failure.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/commands/ctx-wrapup.ts, line 474:

<comment>When a historian publishes the chunk and then `onPublished` throws, `lastEnd` still contains the pre-chunk value, so the Partial result reports the wrong wrapped-through message. Refresh `lastEnd` from `getLastCompartmentEndMessage` before formatting this failure.</comment>

<file context>
@@ -451,11 +452,28 @@ export async function runPiWrapup(
 				}
 
+				if (chunkFailure) {
+					failure = `historian chunk failed (${chunkFailure}); wrapped up through message ${lastEnd}. Run /ctx-wrapup again to continue.`;
+					break;
+				}
</file context>

Suggested change

	failure = `historian chunk failed (${chunkFailure}); wrapped up through message ${lastEnd}. Run /ctx-wrapup again to continue.`;
	lastEnd = getLastCompartmentEndMessage(deps.db, sessionId);
	failure = `historian chunk failed (${chunkFailure}); wrapped up through message ${lastEnd}. Run /ctx-wrapup again to continue.`;

[cubic-dev-ai]

P2: When a historian rejects with an empty error message, chunkFailure is "", so this check treats the failed chunk as successful progress. Track failure with a boolean or an object instead of using the formatted message as the sentinel.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/commands/ctx-wrapup.ts, line 473:

<comment>When a historian rejects with an empty error message, `chunkFailure` is `""`, so this check treats the failed chunk as successful progress. Track failure with a boolean or an object instead of using the formatted message as the sentinel.</comment>

<file context>
@@ -451,11 +452,28 @@ export async function runPiWrapup(
+					}
 				}
 
+				if (chunkFailure) {
+					failure = `historian chunk failed (${chunkFailure}); wrapped up through message ${lastEnd}. Run /ctx-wrapup again to continue.`;
+					break;
</file context>

[cubic-dev-ai]

P3: Each execution leaves the file-backed test database in the shared temporary directory. Track the generated path and remove the database artifacts in finally, preferably by creating a temporary directory and deleting that directory after closing all connections.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/commands/ctx-wrapup.test.ts, line 45:

<comment>Each execution leaves the file-backed test database in the shared temporary directory. Track the generated path and remove the database artifacts in `finally`, preferably by creating a temporary directory and deleting that directory after closing all connections.</comment>

<file context>
@@ -39,6 +41,14 @@ function createDb(): Database {
 }
 
+function createFileDb(name: string): Database {
+	const path = join(tmpdir(), `${name}-${crypto.randomUUID()}.db`);
+	const db = new Database(path);
+	initializeDatabase(db);
</file context>

[cubic-dev-ai]

P3: Delete the temporary SQLite file after closing the database; this test creates a UUID-named file under tmpdir(), but closeQuietly(db) only closes the handle, so repeated runs leave pi-wrapup-busy-release-*.db files behind.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/commands/ctx-wrapup.test.ts, line 333:

<comment>Delete the temporary SQLite file after closing the database; this test creates a UUID-named file under `tmpdir()`, but `closeQuietly(db)` only closes the handle, so repeated runs leave `pi-wrapup-busy-release-*.db` files behind.</comment>

<file context>
@@ -276,6 +286,54 @@ describe("Pi /ctx-wrapup", () => {
+			expect(result).toContain("## Magic Wrapup");
+			expect(getWrapupInProgressState(db, sessionId)).toBeNull();
+		} finally {
+			closeQuietly(db);
+		}
+	});
</file context>

[magic-alfonso]

Thanks — the diagnosis is right, and we verified it at source: Pi parks the historian IIFE in inFlightHistorian with no .catch, and a SQLITE_BUSY from the inner finally lease DELETE rejects that promise. releaseCompartmentLease is a bare DELETE with only busy_timeout behind it (the dreamer's guarded-write retry is not on this path), and on the Pi CLI an unhandled rejection is fatal (Pi has only an uncaughtException handler, which exits). The spawn-side containment is the right shape: best-effort DELETE, TTL steal at 5 minutes in acquireCompartmentLease, and the trigger does not read the lease table, so the next fire no-ops at acquire until expiry. Partial resume via getLastCompartmentEndMessage is consistent with what OpenCode's orchestrator already does.

Asks before merge:

1. if (chunkFailure !== null) — an empty Error.message is falsy and skips the Partial path (greptile's point, and cubic's suggestion of a boolean or object sentinel is the cleaner shape).
2. Clean up the file-backed test database. createTestTempDir in packages/plugin/src/shared/test-temp-dir.ts is the repo helper; the test currently leaves pi-wrapup-busy-release-*.db plus WAL/SHM behind in the temp dir on every run.
3. The contention test fails on master, but for the wrong reason: the red stack is the mock's releaseCompartmentLease(..., "stale-holder") throwing inside the historian mock, before wrapup's own finally DELETE runs — the blocker is rolled back by then. Hold the BEGIN IMMEDIATE across the mock's return so the finally release is what throws; as written the test passes without exercising the path it names.
4. The new .finally in spawnPiHistorianRun has the same catch-before-finally shape (cubic's point): a throw from onStatusChange or unregister inside it still escapes because the .catch has already run. Wrap the finalizer body in a try/catch.
5. In the Partial message, lastEnd is stale when the chunk published and then onPublished threw — refresh it from getLastCompartmentEndMessage before formatting (cubic's suggestion is exactly right).
6. Please correct the comments claiming OpenCode's startCompartmentAgent .catch covers this: it sits before its .finally, so it does not cover a busy lease release either — OpenCode has the same hole, and wrapup calls runCompartmentAgent directly without going through it. What contains a busy-starved release is the try/catch around the DELETE, which is the part this PR gets right. We will fix the OpenCode side separately.

No PARITY.md row needed: OpenCode wrapup already returns Partial, and production runPiHistorian already swallows like runCompartmentAgent; the guarded lease release is Pi going further than OpenCode, which is fine.

Pi suite on this head: 918 pass / 0 fail.