refactor: extract ResourceScope and KeyedSerializer helpers

[ThomasK33]

Summary

This change addresses concrete maintainability issues raised during the
Effect-library discussion without adopting Effect or rewriting the app. It
extracts two small, dependency-free helpers and refactors the two agreed
callsites:

• ResourceScope for renderer resource cleanup with deterministic LIFO
  release, aggregated ResourceScopeCloseError failure reporting, and
  idempotent close().
• KeyedSerializer<K> for per-key serialized work, replacing the module-local
  appendQueues Map in src/storage/artifactManifest.ts.

The renderer dispose path is now refactored onto a per-lifecycle
ResourceScope so that each acquired resource (server, browser,
browserContext, page) registers its release at acquisition time and the
boot-failure rollback uses the same close + log helper as dispose(). Renderer
public dispose() remains best-effort; failures are logged through
this.logger.warn with { name, error } per release and dispose() resolves
successfully. The pre-existing boot() after dispose() contract (covered by
test/integration/renderer-backend.test.ts) is preserved.

Notable choices and deviations from the original plan

• Per-lifecycle ResourceScope instead of a single class-level scope,
  because the integration test "recovers state after dispose and re-boot"
  exercises a second boot. The plan explicitly anticipated this fallback.
• ResourceScopeCloseError extends Error with a failures: readonly ResourceScopeFailure[] field instead of AggregateError. The project
  targets ES2024 so AggregateError is available; the choice is about API
  shape: a typed failures array carrying { name, error } per release is
  clearer for callers than AggregateError's untyped errors.
• EventLog and PTY ingestion queues are not refactored: their semantics
  intentionally differ (poisoning vs. recovery) and would be silently
  changed by a generic per-key serializer. Inline comments now document
  why.

Cancellation follow-up

The plan calls for a separate GitHub issue threading AbortSignal through host wait/poll paths. Filed as #84 with the ready-for-agent label.

Validation

mise run format-check         # all files clean
mise run lint                 # 0 warnings, 0 errors
mise run typecheck            # no errors
npm run verify                # 1175/1175 tests, build green, smoke install green

Plus the renderer-forced manual smoke from the plan, against an isolated
AGENT_TTY_HOME, with a ps baseline:

create  --renderer ghostty-web   # ok: true, sessionId returned
snapshot --renderer ghostty-web   # ok: true, capturedAtSeq, cols
screenshot --renderer ghostty-web # ok: true, 5524-byte PNG written
destroy --force --renderer ghostty-web
gc      --renderer ghostty-web    # removed: 1
ps diff before/after              # PASS: no orphan Chromium/Playwright procs

ADRs

• Renamed docs/adr/0001-use-release-it-only-for-release-prep.md to 0002-...
  to resolve the duplicate 0001 numbering.
• Added docs/adr/0003-renderer-dispose-swallows-cleanup-failures.md
  documenting the best-effort dispose contract and the per-lifecycle scope
  decision.

Files

• src/util/resourceScope.ts, src/util/keyedSerializer.ts (+ unit tests)
• src/storage/artifactManifest.ts — uses KeyedSerializer<string>
• src/renderer/ghosttyWeb/backend.ts — per-lifecycle ResourceScope,
  closeResourceScopeAndLog()
• src/host/eventLog.ts, src/host/hostMain.ts — clarifying comments only
• test/unit/storage/artifactStorage.test.ts — concurrent-writer regression
• test/unit/renderer/ghosttyWebBackend.test.ts — dispose-cleanup logging test
• docs/adr/0002-use-release-it-only-for-release-prep.md (renamed)
• docs/adr/0003-renderer-dispose-swallows-cleanup-failures.md (new)

---

📋 Implementation Plan

Plan: ResourceScope + KeyedSerializer Refactor

Goal

Address the concrete maintainability issues surfaced during the Effect-library discussion without adopting Effect or rewriting the app.

The agreed approach is targeted, pragmatic, dependency-free helper extraction:

1. Introduce a small ResourceScope utility for renderer resource cleanup.
2. Introduce a small KeyedSerializer<K> utility for per-key serialized work.
3. Refactor only the agreed callsites.
4. Defer broader cancellation/AbortSignal work into a concrete GitHub issue.

Non-goals

• No Effect adoption.
• No Zod-to-Effect Schema migration.
• No CLI JSON envelope changes.
• No public protocol/schema changes.
• No broad hostMain.ts cancellation rewrite in this stack.
• No error-channel rewrite; CliError remains as-is.
• No CONTEXT.md changes; the resolved terms are implementation patterns, not domain language.

Evidence and constraints

• Existing docs use a single root CONTEXT.md; the current glossary is domain-focused (Session, Event Log, Render Wait, Snapshot Capture, etc.). Utility terms such as ResourceScope, serializer, queue, and cleanup policy do not belong there.
• Existing ADR numbering is currently duplicated:
  • docs/adr/0001-adopt-oxc-lint-format-tooling.md
  • docs/adr/0001-use-release-it-only-for-release-prep.md
• Git history shows the Oxc ADR landed before the release-it ADR, so the Oxc ADR should remain 0001; the release-it ADR should be renamed to 0002 in PR2.
• gt is installed (1.8.5) but this repo is not yet initialized for Graphite; PR1 prep should run gt repo init before creating the stack and verify whether it creates a tracked .graphite_repo_config. If it does, include that file deliberately in PR1 or document why it is local-only.
• src/renderer/ghosttyWeb/backend.ts currently performs renderer cleanup via cleanupHandles(), manually nulling page, browserContext, browser, server, and serverOrigin, then closing resources with repeated try/catch blocks that swallow individual cleanup failures.
• cleanupHandles() is called from both renderer boot-failure rollback and normal dispose.
• src/storage/artifactManifest.ts currently uses a module-local appendQueues: Map<string, Promise<void>> with an identity-check .finally() cleanup to serialize manifest appends per session directory.
• src/host/eventLog.ts and src/host/hostMain.ts also use promise chains, but their semantics are intentionally different and should not be abstracted in this change.

Resolved design decisions

#	Decision
Q1	Direction: targeted pragmatic helpers, no new runtime dependency.
Q2	Scope: renderer dispose + storage queue. Cancellation gaps become a separate issue.
Q3	ResourceScope collects release failures and throws ResourceScopeCloseError from close().
Q4	Only KeyedSerializer<K> is extracted; EventLog and PTY queues stay inline with comments.
Q5	ResourceScope API: add(name, release) + close(). close() returns the same in-flight/completed result; add() after close throws.
Q6	KeyedSerializer<K>.run<T>(key, op) is generic and preserves current cascading-failure semantics.
Q7	Renderer public dispose() remains best-effort: ResourceScopeCloseError failures are logged and swallowed at the public boundary.
Q8	Renderer cleanup failures are logged through this.logger.warn, not console or the event log.
Q9	Add helper unit tests plus one concurrent-writer artifact manifest test.
Q10	Validation is tests-only CI plus a manual isolated CLI smoke/orphan-process check; no new dogfood proof bundle.
Q11	Land as two Graphite-stacked PRs.
Q12	Add one ADR for best-effort renderer dispose; use inline comments for serializer semantics.
Q13	File a concrete cancellation follow-up issue during PR2 prep.
Q14	Use a single class-level renderer ResourceScope; accept tighter post-completion dispose semantics.
Q15	Rename the duplicate release-it ADR to 0002 in PR2; new renderer dispose ADR becomes 0003.

Architecture notes

ResourceScope

Add src/util/resourceScope.ts with a minimal class and named close error:

export interface ResourceScopeFailure {
  readonly name: string;
  readonly error: unknown;
}

export class ResourceScopeCloseError extends AggregateError {
  public readonly failures: readonly ResourceScopeFailure[];
}

export class ResourceScope {
  private releases: Array<{
    name: string;
    release: () => Promise<void> | void;
  }> = [];
  private closed = false;
  private closing: Promise<void> | null = null;

  add(name: string, release: () => Promise<void> | void): void;
  close(): Promise<void>;
}

Required semantics:

• Release callbacks run in LIFO order.
• Every registered release is attempted even if earlier releases fail.
• If one or more releases fail, close() rejects with ResourceScopeCloseError.
• ResourceScopeCloseError preserves both the original errors and their resource names via a failures array, so callsites can log failure.name and failure.error without parsing the error message.
• The aggregate error message includes the failed resource names for debugging.
• close() is idempotent and returns the same promise/result for concurrent or later callers.
• Failed releases are not re-run on a later close().
• add() after close throws via the repo's existing invariant/assertion helper.
• Assertions should guard non-obvious assumptions: non-empty resource names and function release callbacks.
• Implementation must verify the repo's TypeScript/lib target supports built-in AggregateError. If not, make ResourceScopeCloseError extend Error while keeping the same failures API; do not change tsconfig just for this helper.

KeyedSerializer<K>

Add src/util/keyedSerializer.ts with a minimal class:

export class KeyedSerializer<K> {
  private readonly chains = new Map<K, Promise<unknown>>();

  run<T>(key: K, operation: () => Promise<T>): Promise<T>;
}

Required semantics:

• Operations for the same key run sequentially.

• Operations for different keys may run concurrently.

• The per-key chain is removed using the same identity-check cleanup pattern currently in appendArtifact().

• Preserve the current cascading-failure semantics: if an earlier operation in a still-active chain rejects, later queued operations in that same chain inherit the rejection and do not run.

• Return the .finally(...)-tracked promise, not the pre-finally promise, so the tracked chain cannot reject unobserved.

• The implementation shape should be:

  const previous = this.chains.get(key) ?? Promise.resolve();
  const queued = previous
    // Single-argument then is intentional: predecessor rejection should skip this operation while the active chain drains.
    .then(() => operation())
    .finally(() => {
      if (this.chains.get(key) === queued) {
        this.chains.delete(key);
      }
    });
  this.chains.set(key, queued);
  return queued;

• After the chain drains and the map entry is removed, a later operation for the same key starts fresh, including after a rejection.

• The helper should include a short code comment explaining why the single-argument .then(() => operation()) is intentional.

Renderer dispose policy

Refactor src/renderer/ghosttyWeb/backend.ts so cleanup registration happens at acquisition time:

• Add a class-level ResourceScope field.

• Keep public dispose() memoized with a disposePromise, but do not reset that promise after completion:

  this.disposePromise ??= this.disposeInternal();
  return this.disposePromise;

  This prevents concurrent dispose races and avoids re-running dispose after completion.

• Memoize the scope-close/log helper separately so boot-failure rollback and a later public dispose() do not re-log the same cached ResourceScopeCloseError:

  private resourceScopeClosePromise: Promise<void> | null = null;
  
  private closeResourceScopeAndLog(): Promise<void> {
    this.resourceScopeClosePromise ??= this.closeResourceScopeAndLogInternal();
    return this.resourceScopeClosePromise;
  }

• Before committing to the single class-level scope, implementation must inspect callers/tests to confirm no second lifecycle is supported after either dispose() or boot failure. If rebooting a disposed/failed backend is supported or tested, switch to a per-lifecycle scope instead.

• After acquiring server, register closeServer(server).

• After acquiring browser, register browser.close().

• After acquiring browserContext, register browserContext.close().

• After acquiring page, register page.close() guarded by !page.isClosed().

• Release closures must capture local resource variables (server, browser, browserContext, page) rather than reading mutable this.* fields.

• ResourceScope handles release only; it does not reset renderer state. Explicitly null/reset this.page, this.browserContext, this.browser, this.server, this.serverOrigin, and this.isBooted on both boot-failure rollback and normal dispose.

• bootInternal() failure rollback closes the scope through a helper that catches/logs ResourceScopeCloseError, preserves the boot failureReason, and then rethrows the boot failure.

• dispose() closes the same scope through that helper and then performs the existing state reset except it must not clear disposePromise.

• Public dispose behavior stays best-effort: release errors are logged and swallowed.

• The previous redundant behavior where dispose() could re-run post-completion is intentionally tightened: subsequent dispose calls reuse the same completed dispose promise.

Renderer dispose logging

Add a private helper such as closeResourceScopeAndLog():

• Calls await this.resourceScope.close().
• Catches ResourceScopeCloseError.
• Logs each failure via this.logger.warn with failure.name and failure.error.
• Catches/logs unexpected non-scope errors defensively.
• Does not append renderer cleanup failures to the session event log.
• Does not use console.error.

Queue/serializer callsites

Refactor only src/storage/artifactManifest.ts:

• Replace the module-local appendQueues Map and inline queue management with a module-local KeyedSerializer<string>.
• appendArtifact() should call appendSerializer.run(resolvedSessionDir, async () => { ... }).
• Keep the current validation order: resolve session dir, derive expected session ID, validate entry, then queue the read/write operation.

Do not abstract these queues:

• src/host/eventLog.ts write queue: intentionally poisoning, because event-log sequence gaps would break canonical replay truth.
• src/host/hostMain.ts PTY ingestion queue: intentionally recovers after failure and runs operations despite predecessor rejection.

Add concise comments at those two sites documenting why they remain explicit.

Stack plan

PR1: Utility helpers

Branch created with Graphite after initializing the repo if needed.

Contents:

• src/util/resourceScope.ts
• src/util/keyedSerializer.ts
• test/unit/util/resourceScope.test.ts
• test/unit/util/keyedSerializer.test.ts

Acceptance criteria:

• ResourceScope tests prove:
  • LIFO release order.
  • All releases are attempted after failures.
  • ResourceScopeCloseError includes original errors and failed resource names.
  • Concurrent close() callers receive the same result.
  • A second close() after completion returns the same result and does not re-run releases, including after failed releases.
  • add() after close throws.
• KeyedSerializer tests prove:
  • Same-key operations run sequentially.
  • Different-key operations can overlap.
  • Rejections propagate to callers.
  • Cascading failure inside an active chain is preserved.
  • A later operation after the chain drains starts fresh, including after a rejection.
  • Generic return values are preserved.
• No existing production callsites are changed in PR1.

Validation gate:

mise run format-check
mise run lint
mise run typecheck
npm run test -- test/unit/util/resourceScope.test.ts test/unit/util/keyedSerializer.test.ts
mise run ci

PR2: Callsites, ADR, and follow-up issue

Stacked on PR1 with Graphite.

Contents:

• Refactor src/renderer/ghosttyWeb/backend.ts to use ResourceScope.
• Refactor src/storage/artifactManifest.ts to use KeyedSerializer<string>.
• Add clarifying comments to the EventLog and PTY ingestion queues.
• Add one concurrent-writer test to test/unit/storage/artifactStorage.test.ts.
• Rename docs/adr/0001-use-release-it-only-for-release-prep.md to docs/adr/0002-use-release-it-only-for-release-prep.md.
• Add docs/adr/0003-renderer-dispose-swallows-cleanup-failures.md.
• File the cancellation follow-up GitHub issue.

Acceptance criteria:

• Renderer cleanup still closes page, browser context, browser, and server in reverse acquisition order.
• Boot-failure rollback still releases partially acquired resources and still resets/nulls renderer state fields.
• Normal dispose still resets/nulls renderer state fields.
• Implementation confirms no second lifecycle is supported after either dispose() or boot failure; if it is supported/tested, the renderer uses a per-lifecycle scope instead of a single class-level scope.
• Public dispose() still resolves successfully when cleanup releases fail, but now logs the failures once, including if boot-failure rollback already closed/logged the scope.
• No renderer cleanup failure is written to the session event log.
• appendArtifact() remains serialized per session directory.
• Concurrent artifact appends for the same session preserve all entries without lost writes.
• EventLog and PTY queues retain their existing semantics.
• ADR numbering is repaired so the existing release-it ADR is 0002 and the new renderer dispose ADR is 0003.
• The cancellation follow-up issue is concrete enough to be labeled ready-for-agent.

Validation gate:

mise run format-check
mise run lint
mise run typecheck
npm run test -- test/unit/storage/artifactStorage.test.ts
mise run ci

Manual smoke / dogfooding gate:

Use an isolated home and avoid mutating ~/.agent-tty. The smoke must include a renderer-backed operation (snapshot or screenshot), because create/run alone may not boot GhosttyWebBackend. Explicitly force ghostty-web with the global --renderer ghostty-web option, or first verify it is already the default in the current workspace.

Capture a process baseline first so the orphan check does not fail on unrelated developer/CI browser processes.

AGENT_TTY_HOME="$(mktemp -d)"
export AGENT_TTY_HOME
BEFORE_PS="$(mktemp)"
AFTER_PS="$(mktemp)"
ps -eo pid=,command= | grep -iE 'chromium|playwright' | grep -v grep | sort > "$BEFORE_PS" || true
CREATE_JSON="$(npx tsx src/cli/main.ts --renderer ghostty-web create --json -- /bin/sh -lc 'printf "hi\\n"; sleep 30')"
SESSION_ID="$(node -e 'const data = JSON.parse(process.argv[1]); console.log(data.result.sessionId);' "$CREATE_JSON")"
npx tsx src/cli/main.ts --renderer ghostty-web snapshot "$SESSION_ID" --json
npx tsx src/cli/main.ts --renderer ghostty-web screenshot "$SESSION_ID" --json
npx tsx src/cli/main.ts --renderer ghostty-web destroy "$SESSION_ID" --force --json
npx tsx src/cli/main.ts --renderer ghostty-web gc --json
ps -eo pid=,command= | grep -iE 'chromium|playwright' | grep -v grep | sort > "$AFTER_PS" || true
diff -u "$BEFORE_PS" "$AFTER_PS"

Expected result:

• create, snapshot, screenshot, destroy, and gc succeed and emit valid JSON.
• snapshot/screenshot exercise the ghostty-web renderer path that owns GhosttyWebBackend cleanup.
• No new Playwright/Chromium process remains after cleanup compared with the pre-smoke baseline.

No dogfood proof bundle is required because this is an internal refactor with focused helper tests and full CI/e2e coverage. If smoke testing reveals a renderer lifecycle regression, stop and either add a targeted renderer regression test or split the renderer refactor into a smaller follow-up.

Cancellation follow-up issue draft

Create a GitHub issue during PR2 prep, after PR1 has landed or while PR2 is stacked on it.

If gh authentication or labels are unavailable during implementation, do not block PR2. Instead, include this issue draft in the PR description or final implementation summary and call out the filing blocker.

Suggested title:

Thread AbortSignal through host waits and polling paths

Suggested labels:

• ready-for-agent

Suggested body:

## Problem

Several host-side waits and polling loops use raw `setInterval` / `setTimeout` without a shared cancellation contract. This makes timeout, shutdown, and future interruption behavior harder to reason about, especially around renderer waits, idle polling, and run-completion observation.

Known areas to inspect:

- `src/host/hostMain.ts` idle-timeout polling.
- `src/host/hostMain.ts` wait/render polling flows.
- `src/host/runCompletionSentinel.ts` polling and completion observation.
- RPC paths that wait on host/renderer work.

## Context

The resource-cleanup refactor introduces a small `ResourceScope` helper for deterministic release. Cancellation work should integrate with that release model rather than adding parallel ad hoc cleanup paths.

## Out of scope

- Adopting Effect or another async runtime.
- Rewriting CLI error handling.
- Changing public JSON envelopes or protocol schemas.
- Reworking OS signal handling unless required for the minimal `AbortSignal` path.

## Acceptance criteria

- Relevant host wait/poll operations accept or derive an `AbortSignal`.
- Aborted operations release registered resources and timers deterministically.
- Non-aborted callers retain current behavior.
- Add at least one targeted test proving abort behavior for a host wait or polling path.
- Full CI passes.

ADR 0003 draft

Suggested file: docs/adr/0003-renderer-dispose-swallows-cleanup-failures.md

# Renderer dispose swallows cleanup failures at the public boundary

Renderer cleanup is best-effort at the public `dispose()` boundary: page, browser context, browser, and local server release failures are logged, but `dispose()` still resolves. Cleanup continues through every registered resource and lower-level cleanup helpers may report a named `ResourceScopeCloseError`, but callers should not have to handle dispose rejection during shutdown or boot-failure rollback.

This preserves the previous shutdown contract while making cleanup failures visible through the logger. Propagating dispose failures would make shutdown paths noisier and could mask the original error that triggered cleanup.

Advisor review incorporated

Advisor feedback was folded into this plan before approval:

• KeyedSerializer now returns the tracked .finally(...) promise to avoid unobserved rejections.
• ResourceScope now exposes a named ResourceScopeCloseError with resource-name-preserving failures.
• Renderer public dispose() keeps a memoized disposePromise and does not reset it after completion.
• The scope-close/log helper is memoized too, preventing duplicate logs across boot-failure rollback plus later dispose.
• Renderer state nulling/resetting is called out separately from resource release, including the disposePromise exception and boot failureReason preservation.
• Implementation must verify no second lifecycle is supported after dispose or boot failure before committing to a single class-level scope.
• The manual smoke now forces or verifies ghostty-web, includes snapshot and screenshot, and compares against a pre-smoke process baseline.
• Graphite initialization and GitHub issue filing have explicit fallback/verification notes.

Risks and mitigations

Risk	Mitigation
ResourceScope changes renderer dispose behavior unexpectedly.	Keep public dispose best-effort; run full CI and renderer-forced manual smoke with before/after process baseline.
Cleanup failure logging pollutes JSON output.	Use existing logger, not console; respect log-level plumbing.
KeyedSerializer accidentally changes manifest append semantics.	Preserve current cascading-failure behavior and add helper + concurrent append tests.
EventLog queue is over-abstracted and loses poisoning semantics.	Do not refactor it; add a clarifying comment only.
PTY ingestion queue is over-abstracted and loses recovery semantics.	Do not refactor it; add a clarifying comment only.
ADR rename breaks references.	Repo search found no references to the duplicate release-it ADR filename; still re-run grep before PR2 commit.
GitHub issue filing is blocked by auth or missing labels.	Do not block PR2; include the issue body in the PR description or final summary and state the blocker.
Graphite setup is missing or creates tracked config unexpectedly.	Run gt repo init before creating the stack; check whether .graphite_repo_config is tracked and include/document it deliberately; avoid manual branch surgery unless gt fails.

Handoff notes for implementation mode

• Use minimal code and match existing style: 2-space indentation, single quotes, semicolons, strict TypeScript, ESM .js imports.
• Prefer existing assertion helpers from src/util/assert.ts.
• Read the touched files before editing in implementation mode; this plan intentionally records design intent, not a substitute for fresh local inspection.
• Use isolated absolute AGENT_TTY_HOME for all CLI smoke testing.
• Do not create PRs unless explicitly asked. If PRs are requested, use gt for the stack and include the required mux-generated footer in PR bodies after checking $MUX_MODEL_STRING and $MUX_THINKING_LEVEL.

---

Generated with mux • Model: anthropic:claude-opus-4-7 • Thinking: max

[ThomasK33]

/coder-agents-review

[ThomasK33]

/coder-agents-review

[coder-agents-review]

The helper extraction is well-done. ResourceScope and KeyedSerializer are small, dependency-free, and well-tested. The per-lifecycle scope deviation from the plan is the right call for the boot-after-dispose contract, and the ADR documents it clearly. The KeyedSerializer preserves cascading-failure semantics faithfully. The hostMain.ts and eventLog.ts clarifying comments are accurate.

The lifecycle management in disposeInternal has five findings at P2, all in the dispose/boot coordination area. Four are regressions from the old cleanupHandles behavior: (1) cleanup failure errors are silently eaten by JSON.stringify and produce "error":{} in operator logs, (2) field nulling is deferred to after scope close instead of before (old code nulled synchronously first), (3) disposePromise becomes a stale resolved Promise when scope is null (synchronous async body), and (4) scope.add() invariant during a boot+dispose race orphans the Chromium process. The fifth is that closeResourceScopeAndLog's logger.warn calls are unguarded, so an EPIPE during shutdown rejects dispose, violating the ADR.

Five P2, seven P3, five Nit.

Process notes: the plan specified two Graphite-stacked PRs with separate validation gates; this delivers everything in one commit. The commit message claims "TDD red-green-refactor" in a single atomic commit. The PR description states ResourceScopeCloseError extends Error to "avoid changing tsconfig," but the target is ES2024 and AggregateError has been available since ES2021.

"The cleanup failure message includes the resource names. Too bad it strips the actual errors before printing them." (Leorio)

---

src/renderer/ghosttyWeb/backend.ts:2204

P2 [DEREM-4] When this.resourceScope is null (dispose before boot, or after a boot failure where the catch block already closed the scope), disposeInternal() contains no await. The entire body, including the finally block that sets this.disposePromise = null, executes synchronously inside this.disposeInternal(). The outer assignment in dispose() then overwrites the null:

this.disposePromise = this.disposeInternal();
//                    ^-- runs synchronously, finally sets this.disposePromise = null
//  ^-- assignment overwrites null with the returned resolved Promise

Sequence: (1) dispose() when scope is null; disposePromise = stale resolved Promise. (2) boot(). Awaits the stale promise (resolves immediately). Boots. Acquires resources. (3) dispose(). Checks this.disposePromise !== null (stale). Awaits it (resolves immediately). Returns. Resources not cleaned up.

The old code avoided this because cleanupHandles() was always called (even with null handles), and await cleanupHandles() always yields at least once (async function boundary), so the finally block ran after the outer assignment.

Fix: clear disposePromise in dispose() after the await, not inside disposeInternal()'s finally.

(Takumi P2)

🤖

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

Addressing the remaining finding from the top-level review body in 42b387b:

DEREM-4 — fixed. disposePromise is now reset by dispose() itself, after the await, instead of inside disposeInternal()'s finally:

public async dispose(): Promise<void> {
  if (this.disposePromise !== null) {
    await this.disposePromise;
    return;
  }

  this.disposePromise = this.disposeAfterBoot();
  try {
    await this.disposePromise;
  } finally {
    // DEREM-4: clear here, not inside disposeInternal()'s finally. When
    // disposeInternal runs synchronously (no scope to close), its finally
    // would otherwise execute before the outer assignment lands and leave
    // a stale resolved Promise pinned on this.disposePromise.
    this.disposePromise = null;
  }
}

The synchronous-disposeInternal scenario (scope is null, no awaits inside disposeInternal) no longer pins a stale resolved Promise on this.disposePromise, so a follow-up boot() + dispose() cycle correctly cleans up the new resources.

The "recovers state after dispose and re-boot" integration test still passes, and the new ordering interacts correctly with DEREM-5's await this.bootPromise since disposePromise is set synchronously before that await.

Other findings from the body (DEREM-2/3/5/7) and the inline comments (DEREM-1, 6, 8–17) are addressed in the same commit and replied inline on each resolved thread.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

All 17 R1 findings addressed cleanly in 42b387b. The fixes are well-traced (DEREM-N comments at each change site), and the verification panel confirmed correctness of the critical lifecycle changes: field-nulling-before-await (DEREM-3), disposePromise reset in dispose() (DEREM-4), disposeAfterBoot serialization (DEREM-5), safeWarn wrapper (DEREM-7), and loggedScopes dedup (DEREM-8).

Two new P3s, one P4. The P3s are a pre-existing race narrowed by this PR and a missing test for the safeWarn guard. Neither is blocking.

CI has one e2e failure (test-e2e 3/3). Worth checking whether it is related to the dispose changes or a flaky upstream.

"Good fights from R1, properly finished." (Hisoka)

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

All 3 R2 findings addressed in 8297296. The recordUnexpectedFailure fix (DEREM-18) correctly gates bootPromise clearing behind wasBooted, closing the mid-boot race. The safeWarn test (DEREM-19) and loggedScopes dedup test (DEREM-20) pin their respective guards.

One new P3 from Bisky: the disposeAfterBoot boot-wait branch (DEREM-5 fix) has no unit test coverage because all three dispose tests leave bootPromise null. One Nit on a test comment.

Three rounds, 25 findings, 20 addressed, 3 dropped, 2 open. The remaining open items are a test coverage gap and a comment nit. The code is correct and the lifecycle contracts hold.

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

Both R3 findings addressed in fec84fc. The boot-wait test (DEREM-24) correctly parks dispose on a pending bootPromise and verifies ordering. The dedup test comment (DEREM-25) now accurately describes the mechanism.

One new P3: the sibling path where bootPromise rejects is untested. The try/catch in disposeAfterBoot is the only thing preventing a boot-failure rejection from breaking the ADR 0003 dispose contract.

Four rounds, 26 findings, 22 addressed, 3 dropped, 1 open. The open item is a test gap for the boot-rejection path. The production code is correct; the lifecycle contracts hold across all traced interleavings.

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

DEREM-26 addressed in 5058a29. The boot-rejection test pins the ADR 0003 contract across both boot success and boot failure paths. Three reviewers, zero new findings.

Five rounds complete. 26 findings raised, 23 fixed, 3 dropped. Zero open. The code is clean, the lifecycle contracts are verified, and the test suite covers every defensive guard.

🤖 This review was automatically generated with Coder Agents.