Repository navigation
Recover ownership of a stuck agent session - #996
Conversation
Integrates the reviewed private ownership delta (8ecd5cf1..bb59899f) onto main: one strict unversioned ownership record with a nullable forced-recovery marker, `xmd session inspect/recover --force` administration held in host assembly, joined cancellation of pending ownership I/O, and the architecture rule that prerelease internal persistence has one current format with no compatibility branches, migrations or upgrade paths, manual paths included. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR #996: Recover ownership of a stuck agent session14 files, +1992 / -316 Scope🔴 PR has 2308 lines changed. Split into focused PRs. 🟡 2308 lines changed. PRs under 400 receive more thorough review. 🟡 Large PR with no linked issue. Structural🟡 5 console statements. Oxlint structural signals:
SlopOxlint slop signals:
Static AnalysisOxlint: 32 diagnostics across 5 files (13 rules) no-console (6): packages/cli/src/session-ownership.ts, packages/cli/src/cli.ts CorrectnessNo extraneous code patterns detected. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Fable 5.1 first-green review of 9627e8e1 (base 53cdd360).
Verdict: no correctness blocker found. Two concrete duplicate-work simplifications and one export-layout nit are posted inline; none changes accepted behavior.
Correctness. The ownership path keeps its invariants: occupancy is reserved before any I/O, the lease handle and lock are released only after inner scopes settle, the idle write runs only on acknowledged quiescence, and force rereads the exact bytes under the same exclusive() boundary as ordinary acquisition. pending() registers its ensure() before the host call starts, so a cancelled open/lock/write/rename/sync still settles inside its owner and adopts the late result (FR5 covers all six phases, FR6 the first-failure attribution). The parser admits exactly the six current fields, ties release to state, and requires a self-consistent lastForcedRecovery when release is forced (HC13 negative controls). Administration reaches the CLI only through MachineSessionAssembly.ownershipAdministration; hostAcpDependencies still projects no such member (HC14). The thrown-vs-returned asymmetry for an invalid retained record (throw AgentSessionRecoveryRequired at coordinate) is inherited from the base and pinned by the existing CO regression, so it is not a finding here.
Performance. Conditional analysis, no benchmark run and none warranted: per acquisition the new code does one bounded read into a fixed 64 KiB buffer plus one SHA-256 over at most 64 KiB, and publication does exclusive-create, write loop, fsync, rename, directory fsync, the same syscall count as the base's writeTextFile/open/sync/rename/open/sync sequence. useMachineSessions() now builds one host instead of calling useSessionCoordinator() twice, a small win. The one measurable regression is the extra open/read/parse/hash on the recovery-required refusal path (inline comment). If anyone wants numbers, timing coordinate in the CO tests before and after that one-line change is the focused measurement; a broad benchmark would not discriminate.
Minimalism. The record/receipt types, the shared exclusive() and pending() helpers, and the single serializer are the right size for the accepted scope. The inline comments are the only reductions I would make.
Evidence consulted. Required green check and the lint, test-deno, test-node, test-bun, filesystem-contract, smoke, jsr and site jobs pass on this head (run 40426719308). The focused command deno task test packages/runtime/tests/agent-session-coordinator.test.ts packages/cli/tests/agent-session-coordinator.test.ts packages/cli/tests/session-ownership-cli.test.ts packages/cli/tests/cli-help.test.ts packages/cli/tests/repl-sidekick-acp.test.ts packages/cli/tests/repl-sidekick-dispatch.test.ts passed 13 containers / 94 steps on the staged tree 75cf8755, which equals this commit's tree. The xmd-review bot's oxlint signals (no-array-sort, no-shadow, …) come from rules outside the repository's enforced .oxlintrc.json; the enforced lint job passes.
Scope: one-time first-green review. This is not the delivery gate; required delivery verification stays separate.
| } | ||
| } | ||
| if (retained?.state === "active") { | ||
| const observed = yield* read(digest); |
There was a problem hiding this comment.
Redundant second read on the recovery-required path (performance / minimalism).
Line 399 already calls read(digest) and that result carries recordRevision (see line 220), but only .record is kept, so line 414 opens, reads, parses and hashes the same file again under the lock to recover the revision it just discarded.
Consequence: one extra open/read/parse/SHA-256 on every refusal, and the --expect value in the message is computed from bytes read later than the bytes the admission decision used. Under the lock only a non-locking external writer could change them, so this is not a live bug, but the two reads are not provably the same record.
Recommendation (preserves behavior; CO4 and FR3 assert only the error name):
let retained: AgentSessionOwnershipInspection | undefined;
try {
retained = yield* read(digest);
} catch (cause) { /* unchanged */ }
if (retained?.record.state === "active") {
return Err(new AgentSessionRecoveryRequired(
`… --expect ${retained.recordRevision} --force`,
{ keyDigest: digest, recordRevision: retained.recordRevision },
));
}
// then use retained?.record.lastForcedRecovery below| const receipt: AgentSessionForceRecoveryReceipt = { | ||
| keyDigest: request.keyDigest, | ||
| previousRecordRevision: request.expectedRevision, | ||
| recordRevision: revision( |
There was a problem hiding this comment.
Receipt revision re-serializes and re-hashes bytes publish already produced (minimalism).
publish() encodes serializeAgentSessionOwnership(record) at line 226 and writes exactly those bytes; lines 507-508 serialize and encode the same record a second time to hash it for the receipt. The two are equal only because serialization is deterministic, which is true today but is an unstated coupling.
Recommendation: have publish() return revision(bytes) (the hash of what it actually wrote) and use that here:
const recordRevision = yield* publish(request.keyDigest, record);
…
recordRevision,One serialize/encode/hash instead of two, and the receipt's recordRevision is the hash of the written bytes by construction. FR2/FR4 compare later inspections, so behavior is unchanged.
| hasDenoExecutableObserver, | ||
| } from "./deno-executable-observer.ts"; | ||
|
|
||
| export { AgentSessionOwnershipError } from "./agent-session-coordinator.ts"; |
There was a problem hiding this comment.
New exports appended as separate blocks from modules that already have export blocks (minimalism nit).
Lines 185-195 add a second export { … } from "./agent-session-coordinator.ts", a second export type { … } from "./agent-session-coordinator.ts", and a second export { … } from "./deno-agent-session-coordinator.ts", while the originals sit at lines 160-173. Merging AgentSessionOwnershipError, the seven new types and createDenoAgentSessionHost into those existing blocks keeps the module's public surface readable in one place per source file. No behavior change.
The admission decision already reads and hashes the retained record, so coordinate() keeps that inspection instead of reading the file a second time under the lock. The revision a refusal advertises for `--expect` now comes from the same bytes the decision used. publish() returns the revision of the bytes it wrote, so forced recovery reports what it published rather than re-serializing and re-hashing the record; FR2 pins the receipt to the revision a following inspection reads. The runtime's ownership exports merge into the blocks that already name that module.
Recover a stuck agent session with an explicit force operation while retaining its native session identity and history. Ordinary invocations continue to respect an active owner. Forced recovery retains an explicit record of unproved cleanup and unknown outcome.
Internal ownership records use one strict current format. The architecture makes the prerelease policy explicit: change internal persistence in place, with no version compatibility branches, migrations or upgrade adapters, including manual paths.
The integration preserves the merged ACP startup and mount behavior. Fresh focused Deno validation passes 13 containers and 94 steps covering ownership, forced recovery, cleanup, CLI behavior and ACP regressions. Required CI, once-first-green Fable review, runner weight measurement and final metadata verification are handled by the delivery workflow.