Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions .tmp-651d.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import fs from "fs";
const f = "extensions/taskplane/hold-state.ts"; let s = fs.readFileSync(f, "utf8");
const rep = (a, b) => { if (s.split(a).length !== 2) { console.error("ANCHOR", a.slice(0, 80), s.split(a).length); process.exit(1); } s = s.replace(a, b); };
rep(` if (open.length > 0) {
if (existing.status === "held") return noop;
existing.status = "held";
existing.exitReason = \`Held — awaiting ruling on \${open.map((h) => h.escalationId).join(", ")}\`;
existing.endTime = null;
return { changed: true, restore };
}
if (releasedUnacked.length > 0) {
if (existing.status === "running" && !/^Held|^Hold timeout/.test(existing.exitReason)) return noop;
existing.status = "running";
existing.exitReason = \`Ruling \${releasedUnacked.map((h) => h.ruling?.id ?? "?").join(", ")} accepted — worker relaunched\`;
existing.endTime = null;
return { changed: true, restore };
}
return noop;`,
` let want: { status: LaneTaskOutcome["status"]; exitReason: string } | null = null;
if (open.length > 0) {
want = {
status: "held",
exitReason: \`Held — awaiting ruling on \${open.map((h) => h.escalationId).join(", ")}\`,
};
} else if (releasedUnacked.length > 0) {
want = {
status: "running",
exitReason: \`Ruling \${releasedUnacked.map((h) => h.ruling?.id ?? "?").join(", ")} accepted — worker relaunched\`,
};
}
if (!want) return noop;
if (existing.status === want.status && existing.exitReason === want.exitReason) return noop;
existing.status = want.status;
existing.exitReason = want.exitReason;
existing.endTime = null;
return { changed: true, restore };`);
fs.writeFileSync(f, s); console.log("ok");
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- **A released hold left the task badged `held` and let its lane successor show `running` with the wrong telemetry** (#651). Three writers: (1) the strict hold-store write that persists a release (or an open) now projects the bound task's record in the same write — open → `held`, released → `running` with the stale hold-timeout reason cleared — instead of waiting for the next unrelated task-transition persist; (2) resume reconciliation decides "session alive" per **task** from the registry manifests' task ids, so a not-yet-started successor sharing the lane's session name stays `pending` (legacy registries without task ids keep the old session semantics); (3) the dashboard attaches a lane's live worker telemetry to the task named by the lane snapshot — including a row still badged `held` — never to a sibling that merely has `status: running`.
- **Stale `⏸️ Lane held` alert after a fast ruling.** The escalation reaches the
supervisor live, so a ruling is often queued before the worker exits; the hold
loop published \"Lane held\" and then consumed that ruling on its first poll.
Expand Down
11 changes: 10 additions & 1 deletion dashboard/public/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,15 @@ function renderLanesTasks(batch, sessions) {
// so allow a task-status fallback while still avoiding duplicate rows.
const reviewerActive = isReviewerActiveForTask(ls, task);
const telemBadges = task.status !== "pending" ? telemetryBadgesHtml(tel, reviewerActive) : "";
if (ls && ls.workerStatus === "running" && task.status === "running") {
// #651: the lane snapshot names the task its worker is running. Attach the
// lane's live numbers to THAT task (even if its batch-state badge is stale,
// e.g. still `held` right after a ruling), never to a sibling whose badge
// happens to say running.
const snapshotOwnsTask = !ls || !ls.taskId || ls.taskId === task.taskId;
const workerActiveHere =
!!ls && ls.workerStatus === "running" && snapshotOwnsTask &&
(task.status === "running" || (task.status === "held" && ls.taskId === task.taskId));
if (workerActiveHere) {
const elapsed = ls.workerElapsed ? `${Math.round(ls.workerElapsed / 1000)}s` : "";
const tools = ls.workerToolCount || 0;
const ctx = ls.workerContextPct ? `${Math.round(ls.workerContextPct)}%` : "";
Expand All @@ -1005,6 +1013,7 @@ function renderLanesTasks(batch, sessions) {
workerHtml += telemBadges;
workerHtml += `</div>`;
} else if (!ls && tel && task.status === "running") {
// (no snapshot to attribute by — early startup only)
// Running task with telemetry but no lane-state yet (early startup)
const lastTool = tel.lastTool || "";
workerHtml = `<div class="worker-stats">`;
Expand Down
23 changes: 13 additions & 10 deletions extensions/taskplane/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2691,16 +2691,19 @@ export async function executeOrchBatch(
// batch state STRICTLY (throw on failure) before the lane-runner acts on
// them — never the best-effort persistRuntimeState path.
if (!batchState.holds) batchState.holds = [];
const holdStore = createHoldStore(batchState, (reason) =>
persistRuntimeStateStrict(
reason,
batchState,
wavePlan,
latestAllocatedLanes,
allTaskOutcomes,
discoveryRef,
stateRoot,
),
const holdStore = createHoldStore(
batchState,
(reason) =>
persistRuntimeStateStrict(
reason,
batchState,
wavePlan,
latestAllocatedLanes,
allTaskOutcomes,
discoveryRef,
stateRoot,
),
{ outcomes: () => allTaskOutcomes }, // #651: status projection in the same write
);
// TP-029: Track all repo roots encountered during execution.
// Maps repoRoot → repoId (undefined for primary/repo-mode).
Expand Down
87 changes: 82 additions & 5 deletions extensions/taskplane/hold-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import { existsSync, readdirSync, readFileSync } from "fs";
import { join } from "path";
import type { MailboxMessage } from "./types.ts";
import type { LaneTaskOutcome, MailboxMessage } from "./types.ts";
import { isValidMailboxMessage } from "./mailbox.ts";

// ── Types ─────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -634,24 +634,38 @@ export class HoldPersistenceError extends Error {
*/
export function createHoldStore(
owner: { holds?: HoldRecord[] },
/** Durable whole-state write; throws on failure. */
persist: (reason: string) => void,
opts: {
/**
* #651: the live outcome array `serializeBatchState` reads task statuses
* from. When supplied, every commit projects the bound task's status from
* the post-transition hold table in the SAME write, and rolls the
* projection back with the hold table if the write fails.
*/
outcomes?: () => LaneTaskOutcome[];
} = {},
): HoldStore {
const commit = (next: HoldRecord[], reason: string, escalationId: string): void => {
const commit = (next: HoldRecord[], reason: string, record: HoldRecord): void => {
const prev = owner.holds;
owner.holds = next;
const projection = opts.outcomes
? projectHoldStateOntoOutcomes(opts.outcomes(), next, record.taskId)
: { changed: false, restore: () => {} };
try {
persist(reason);
} catch (err) {
owner.holds = prev;
throw new HoldPersistenceError(escalationId, err);
projection.restore();
throw new HoldPersistenceError(record.escalationId, err);
}
};
return {
list: () => (owner.holds ?? []).map((h) => ({ ...h })),
open: (record) => {
const current = owner.holds ?? [];
if (current.some((h) => h.escalationId === record.escalationId)) return; // idempotent
commit([...current, { ...record }], `hold-open:${record.escalationId}`, record.escalationId);
commit([...current, { ...record }], `hold-open:${record.escalationId}`, record);
},
update: (record) => {
const current = owner.holds ?? [];
Expand All @@ -661,12 +675,75 @@ export function createHoldStore(
commit(
upsertHold(current, { ...record }),
`hold-${record.phase}:${record.escalationId}`,
record.escalationId,
record,
);
},
};
}

/**
* #651: project the unit's hold state onto the task outcome that
* `serializeBatchState` reads statuses from. Evaluated against the POST-transition
* hold table (Sage: a unit with two open holds is still held when only one is
* ruled), never against a single record:
* - any OPEN hold binding the task → `held`
* - none open, some released & unacknowledged → `running` (worker relaunched to deliver)
* - otherwise → untouched
* Never downgrades a terminal outcome. Returns the previous field values so a
* failed persist can restore them (the store does this — projection and hold
* write are one transaction).
*/
export function projectHoldStateOntoOutcomes(
outcomes: LaneTaskOutcome[],
holds: readonly HoldRecord[],
taskId: string,
): { changed: boolean; restore: () => void } {
const noop = { changed: false, restore: () => {} };
const existing = outcomes.find((o) => o.taskId === taskId);
if (!existing) return noop;
if (
existing.status === "succeeded" ||
existing.status === "failed" ||
existing.status === "stalled" ||
existing.status === "skipped"
) {
return noop;
}
const mine = holdsForTask(holds, taskId);
const open = mine.filter((h) => h.phase === "open");
const releasedUnacked = mine.filter(
(h) => h.phase === "released" && h.deliveryState !== "acknowledged",
);
const prev = {
status: existing.status,
exitReason: existing.exitReason,
endTime: existing.endTime,
};
const restore = () => {
existing.status = prev.status;
existing.exitReason = prev.exitReason;
existing.endTime = prev.endTime;
};
let want: { status: LaneTaskOutcome["status"]; exitReason: string } | null = null;
if (open.length > 0) {
want = {
status: "held",
exitReason: `Held — awaiting ruling on ${open.map((h) => h.escalationId).join(", ")}`,
};
} else if (releasedUnacked.length > 0) {
want = {
status: "running",
exitReason: `Ruling ${releasedUnacked.map((h) => h.ruling?.id ?? "?").join(", ")} accepted — worker relaunched`,
};
}
if (!want) return noop;
if (existing.status === want.status && existing.exitReason === want.exitReason) return noop;
existing.status = want.status;
existing.exitReason = want.exitReason;
existing.endTime = null;
return { changed: true, restore };
}

/** Volatile store for tests and legacy callers that run without an engine. */
export function createInMemoryHoldStore(initial: HoldRecord[] = []): HoldStore {
return createHoldStore({ holds: [...initial] }, () => {});
Expand Down
42 changes: 31 additions & 11 deletions extensions/taskplane/resume.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1010,9 +1010,17 @@ export function reconcileTaskStates(
* ruling arrives.
*/
holdBlockedTaskIds: ReadonlySet<string> = new Set(),
/**
* #651: when the registry names the task each alive worker is running,
* "alive" is decided per TASK, not per lane session. Undefined = legacy
* registry without task ids → fall back to session identity.
*/
aliveTaskIds?: ReadonlySet<string>,
): ReconciledTaskState[] {
return persistedState.tasks.map((task) => {
const sessionAlive = aliveSessions.has(task.sessionName);
const sessionAlive = aliveTaskIds
? aliveTaskIds.has(task.taskId)
: aliveSessions.has(task.sessionName);
const doneFileFound = doneTaskIds.has(task.taskId);
const worktreeExists = existingWorktrees.has(task.taskId);

Expand Down Expand Up @@ -1820,10 +1828,17 @@ export async function resumeOrchBatch(
// TP-112/119: Runtime V2 session liveness check only.
// Alive sessions are discovered from the process registry.
const aliveSessions = new Set<string>();
// #651: tasks whose OWN worker is alive. Two tasks on one serial lane share
// the lane's sessionName, so "lane session alive" must not promote a
// not-yet-started successor to running/reconnect.
const aliveTaskIds = new Set<string>();
let manifestsCarryTaskIds = false;
const registry = readRegistrySnapshot(stateRoot, persistedState.batchId);
if (registry) {
for (const manifest of Object.values(registry.agents)) {
if (typeof manifest.taskId === "string" && manifest.taskId) manifestsCarryTaskIds = true;
if (!isTerminalStatus(manifest.status) && isProcessAlive(manifest.pid)) {
if (typeof manifest.taskId === "string" && manifest.taskId) aliveTaskIds.add(manifest.taskId);
aliveSessions.add(manifest.agentId);
// Also add lane session name (without role suffix) so reconciliation
// matches persisted task.sessionName.
Expand Down Expand Up @@ -1874,6 +1889,7 @@ export async function resumeOrchBatch(
doneTaskIds,
existingWorktreeTaskIds,
holdBlockedTaskIds,
manifestsCarryTaskIds ? aliveTaskIds : undefined,
);

// ── 4b. Clear stale session allocation for tasks reconciled as pending ──
Expand Down Expand Up @@ -2118,16 +2134,20 @@ export async function resumeOrchBatch(
outcomes: () => preWaveOutcomes,
discovery: () => preWaveDiscovery,
};
const holdStore = createHoldStore(batchState, (reason) =>
persistRuntimeStateStrict(
reason,
batchState,
holdPersistCtx.wavePlan(),
holdPersistCtx.lanes(),
holdPersistCtx.outcomes(),
holdPersistCtx.discovery(),
stateRoot,
),
const holdStore = createHoldStore(
batchState,
(reason) =>
persistRuntimeStateStrict(
reason,
batchState,
holdPersistCtx.wavePlan(),
holdPersistCtx.lanes(),
holdPersistCtx.outcomes(),
holdPersistCtx.discovery(),
stateRoot,
),
// #651: project status onto whichever outcome set this checkpoint serializes.
{ outcomes: () => holdPersistCtx.outcomes() },
);
// Carry forward unknown fields for roundtrip preservation
if (persistedState._extraFields) {
Expand Down
Loading