Skip to content

fix: harden plugin shutdown and cold-pass isolation - #3

Merged
rmk40 merged 6 commits into
rmk40:mainfrom
kernel-oops:fix/plugin-disposal-cold-pass
Aug 28, 2026
Merged

fix: harden plugin shutdown and cold-pass isolation#3
rmk40 merged 6 commits into
rmk40:mainfrom
kernel-oops:fix/plugin-disposal-cold-pass

Conversation

@kernel-oops

Copy link
Copy Markdown

Summary

  • implement OpenCode's supported asynchronous dispose hook and declare engines.opencode >=1.15.11
  • quiesce cold-pass, incremental, timer, and persistence work before bounded shutdown and SQLite close
  • make summarizer worker cleanup safe across writer-lease handoff using unique ownership and authoritative lease rechecks
  • quarantine malformed sessions by sessionID + timeUpdated so one bad historical row cannot abort and retry the entire cold pass
  • preserve valid recall data across transient metadata transport failures and retry it on later activity

Why

A plugin instance currently owns timers, SQLite state, background distillation, and summarizer workers without exposing lifecycle disposal. When OpenCode disposes a per-directory plugin instance, those resources can remain alive. Separately, one legacy session with an obsolete response shape can abort the complete cold pass; the global retry then scans thousands of healthy sessions again roughly once per minute.

Verification

  • formatting and ESLint pass
  • source and test TypeScript checks pass
  • ESM build and declaration generation pass
  • Vitest: 384 passed, 7 skipped (21 files passed, 2 skipped)
  • focused disposal, lease-handoff, malformed-session, and metadata retry race tests pass repeatedly

Residual constraints

  • the OpenCode SDK does not expose cancellation for already-started requests; shutdown prevents late SQLite work but may wait for an in-flight non-summarizer request
  • a create request that outlives the bounded shutdown window may leave a uniquely owner-tagged remote worker for a later lease holder to remove
  • quarantined revisions are kept in bounded in-memory state and are retried after plugin restart
  • orphan discovery remains bounded to the existing session-list page size

Marc and others added 6 commits August 12, 2026 00:22
…esce/finalize work

- B1: drop the engines.opencode constraint — opencode hard-enforces
  engines and would refuse to load on older hosts; dispose is an
  optional hook, so older hosts simply never call it. peerDependencies
  range unchanged.
- B2: bound dispose(). settleWithin moved to types.ts and wraps both
  the distiller stop and the tracked-operations wait (2.5s each, after
  the summarizer's own 15s bound: <20s worst case). A timeout does NOT
  skip db.close(); detached continuations are fenced by stopped/
  finalized/lease guards before every store read/write. New test proves
  dispose completes when an SDK request never settles.
- B3: fetchMessagePage strictness is opt-in (strict?: boolean). Only
  distiller call sites pass strict:true; the query path keeps the
  documented "non-array data = empty page, ok:true" contract. Restored
  the original recall_messages/recall_context expectations and added
  strict/lenient unit tests.
- B4: owned-worker cleanup is no longer lease-gated. Remote worker
  ownership is a separate authority from the SQLite writer lease;
  delete/abort of an ownedWorkers member goes through a bounded
  ungated runner, while creates, prompts, and orphan sweeps stay
  lease-gated. Test: lease lost mid-batch still deletes the worker.
- B5: stale-writer window closed. Every distiller write batch
  (full/append/cold-pass replace, rollups, deletes) re-verifies
  authoritative ownership via ownsLease() immediately before the write,
  one leaseStatus() read per batch. Test simulates a rival takeover
  between fetch and write.
- I1: ownsLease() no longer self-demotes on its own stale-looking
  heartbeat; ownership is lost only when the row names someone else or
  is gone. Heartbeat freshness is the rival's acquire-side concern.
- I2: the deriveCard quarantine catch is narrowed to data-shape errors
  (MalformedSessionError/TypeError); anything else aborts the pass and
  surfaces in lastError. DistillStatus gains quarantinedCount.
- I3: status() reports the cached lease flag (side-effect-free);
  ownsLease() is reserved for write gates.
- I4: bumped @opencode-ai/plugin to 1.18.23, which declares dispose in
  Hooks, and deleted the local module augmentation. Test-side fallout
  (execute now returns ToolResult) handled with a toolResultText
  narrowing helper.
- I5: disposed-tool calls return the JSON { ok:false, error } shape
  instead of rejecting.
- I6: ownedWorkers entries are removed once cleanup settles (a timed-out
  delete keeps the id owned for the next holder's orphan sweep).
- I7: fetchNewMessages wraps its page walk in the same
  MalformedSessionError conversion as fetchSessionMessages.
- I8: CHANGELOG Unreleased entry for the change-set.

Based on PR rmk40#3 by @kernel-oops with maintainer fixes.
…spose

Round-2 review fixes on the bounded-shutdown change-set.

- Blocker: dispose no longer closes the DB when the tracked-operations
  wait times out. `operations` tracks FOREGROUND tool executions with no
  finalized/lease guards, so a recall paused in an SDK fetch could
  resume into store reads on a closed handle. A distiller-stop timeout
  still closes (its continuations are finalized/lease-guarded); an
  operations timeout skips the close — the process is exiting and the
  derived store is rebuildable, so an unclosed handle is harmless where
  a use-after-close is not. Comment rewritten to state what protects
  each path. New tests: a detached distiller fetch resolved AFTER
  dispose asserts zero post-close DB calls; a foreground tool operation
  outliving the bound asserts the close was skipped and the late
  resumption does not crash.
- recordProgress (coldpass_cursor) is now gated on authoritative
  ownsLease() — it was the one distiller store write outside the
  stale-writer fence — checked only when the progress floor moves, so
  the hot skip path costs nothing extra. The cold-pass "done" transition
  and onColdPassDone() are gated the same way, so a demoted instance
  whose rollup recompute no-oped cannot declare success.
- Page-walk catches in fetchSessionMessages/fetchNewMessages narrowed
  to TypeError (rethrow the rest), matching the quarantine principle.
  The deriveCard catch keeps MalformedSessionError|TypeError with a
  comment acknowledging the accepted tradeoff.
- status().leaseHeld is now derived from the lease row status() already
  reads (holder === instanceId): authoritative AND side-effect-free,
  no extra SQLite read. Stale-writer test expectation updated.
- Inter-page politeness sleeps are tracked and cancelled by quiesce(),
  so a long distillDelayMs cannot keep the runtime alive past dispose.
- ownedWorkerSdk returns the SDK response; deleteOwnedWorker prunes the
  ownedWorkers id only on CONFIRMED success (resp without error) — a
  failed delete keeps the id for this instance's own retry. Comment
  corrected: the next holder's sweep deletes by sentinel regardless.
- db.close() wrapped in try/catch so a throwing close cannot reject
  disposePromise into the host; redundant Promise.allSettled around
  summarizer.stop() replaced with a plain catch.
- Orphan sweep comment documents the accepted winner-deletes-loser's-
  in-flight-worker race (benign: loser's results were lease-gated out).
- fetchSessionMeta classifies a recognizable not-found (v2 `_tag`
  SessionNotFoundError or response.status 404) as absence (null), not a
  transport error.
- CHANGELOG: "opencode >= 1.15.11" softened to "recent opencode
  versions"; bounded-shutdown entry describes the skip-close behavior;
  stale-writer entry now truthfully says every distiller store write is
  fenced (recordProgress included).
Round-3 review fixes.

- Blocker: replace skip-close with deferred close. Skipping leaked the
  SQLite handle when opencode disposes a cached per-directory instance
  without the process exiting (cache eviction/reload). When the
  operations wait times out, dispose now schedules the close behind a
  fresh Promise.allSettled of the stragglers: it fires after
  disposePromise resolved (cannot hang the host), never under a live
  reader, and closes eventually. A truly never-settling straggler
  degrades to the old skip behavior. Test extended: after the parked
  tool resolves, the deferred close fires (closes === 1) with zero
  post-close calls; a comment notes recall_messages is a proxy and the
  sqlite mock proxies every store method, so any tool's post-close
  store call would trip the counter.
- isNotFoundError: session.get's real 404 body is the generic
  NotFoundError discriminated by name:"NotFoundError" (SessionGetErrors
  in the v2 typings), not _tag:"SessionNotFoundError" (another
  endpoint's shape). Added the name check as primary; _tag and
  response.status 404 remain as fallbacks; docstring corrected. New
  tests: name:"NotFoundError" → absence (no lastError, no quarantine,
  card untouched, no retry noise); apiFailure with neither signal →
  transport path with lastError surfaced.
- ownedWorkers retry is now real: drainOwnedWorkers() at the start of
  each batch best-effort re-deletes leftover ids (bounded — each delete
  is already settleWithin-capped), pruning on success and keeping on
  failure. Comment updated to name the actual retry path. Test: a
  failed delete's id is retried and pruned by the next batch's drain,
  nothing leaked.
- TypeError quarantine breadth held deliberately (gpt5's objection
  acknowledged, not converted): both page-walk catches now cross-
  reference the deriveCard accepted-tradeoff comment, which is
  strengthened to name the residual risk explicitly — a TypeError
  regression in distillFields/deriveCard quarantines while the pass
  reports done; bounded by quarantinedCount observability and the
  1000-entry cap; accepted because annotating every field access is
  worse.

Perf (RECALL_PERF=1, after the leaseStatus() reads landed): tier-1 rank
p95 10.45ms (<50 budget), distill+replace p50 0.85ms (<150), ftsSearch
p95 0.96ms (<100), e2e drilled query 20.0ms (<1500), heap 57.9MB (<150).
Round-4 review fix: drainOwnedWorkers was bounded per delete but
unbounded in aggregate — persistent failures grew the retained set one
id per batch (O(n²) deletes across a pass; with timeouts instead of
errors, (k-1)×15s serial waits wedging the pass while holding the
lease).

- Cap the drain at MAX_DRAIN_PER_BATCH (3) ids per batch.
- Give-up counter: after MAX_DELETE_ATTEMPTS (2) failed deletes an id
  is dropped from ownedWorkers and the attempts map (logged). Safe:
  the next holder's sentinel-based orphan sweep reaps it, and
  sentinel-titled sessions are excluded from every recall path
  meanwhile. Attempts are also cleared on successful delete.
- Bounded-backlog policy: a backlog still at/over MAX_DRAIN_PER_BATCH
  after the drain means deletes are persistently failing — skip the
  batch (no new worker) instead of adding to the leak; the give-up
  shrinks the backlog so later batches proceed.
- pluginLog on the deferred-close branch in dispose (count + bound,
  plus a line when the deferred close fires) so a never-settling
  straggler's open handle is diagnosable instead of silent.
- Drain comment states the real bounds (per-batch cap, 2-attempt
  give-up, sentinel sweep backstop).
- Tests: persistent failure → no id delete-attempted more than twice,
  give-up logged; saturated backlog (give-up delayed via the new
  maxDeleteAttempts test affordance) → batches skip worker creation
  (creates capped at 3). Added the missing await summarizer.stop() in
  the round-3 retry test.
@rmk40

rmk40 commented Aug 28, 2026

Copy link
Copy Markdown
Owner

Thanks for this PR — the problem selection is exactly right (the cold-pass poisoning by one malformed session and the missing disposal lifecycle were both real gaps), and the test craftsmanship is above what most of this repo had: the Proxy-based post-close SQLite counter and the rival-store lease probe are patterns we're keeping.

I've reviewed it in depth (three independent review passes), merged main (v2.1.0) into the branch, and pushed maintainer fix commits directly (you had "allow edits by maintainers" on — thanks for that). Summary of what changed and why:

edf4738 — findings from the first review pass:

  • Dropped engines.opencode. opencode hard-enforces engines and would refuse to load the plugin on any older host. dispose is an optional hook — old hosts simply never call it, so no gate is needed. (Also: the type appears in plugin 1.15.11, but there's upstream indication the host may not invoke it until 1.15.13, so the floor was unverifiable anyway.)
  • Bounded dispose(). The unbounded distiller.stop() / operations waits could hang the host's shutdown forever on a never-settling in-flight request — your own residual-constraints note, but the host awaits dispose with no timeout of its own, so "may wait" meant "may wedge opencode on exit." Your settleWithin was the right primitive; it now wraps both phases.
  • fetchMessagePage strictness is now opt-in (strict: true, distiller-only). The non-array throw also changed the query path (recall_messages/recall_context/drill), reverting a documented deliberate decision ("a session with no data returns an empty page rather than an error"). The distiller keeps the strict behavior it needs for quarantine.
  • Owned-worker cleanup is no longer lease-gated. deleteOwnedWorker routed through the lease check, so losing the lease mid-batch skipped deleting a worker this process created — inverting the PR's own goal. Ownership of one's own remote worker is a separate authority from the SQLite writer lease; deletion now gates on ownership only.
  • Authoritative ownsLease() before every distiller store write — closes the stale-writer window where a >TTL-suspended process resumes a fetch and writes after a rival took over.
  • ownsLease predicate fixed to holder-identity only (own stale heartbeat ≠ loss — self-demotion cost a 60s stall), quarantine catch narrowed with quarantinedCount observability, status() made side-effect-free, plugin devDep bumped to 1.18.23 so tsc verifies the real dispose signature (augmentation deleted), guardTool returns the {ok:false} JSON shape, CHANGELOG entry.

45b2805, 953f046, 8d8f733 — findings from re-review of our own fixes (the panel reviewed the fixes as hard as the original): close is deferred (not skipped) when a foreground op outlives the bound — avoids both the use-after-close and a handle leak on instance eviction; recordProgress and the cold-pass "done" transition joined the lease fence; pagination sleeps are cancellable in quiesce(); 404-vs-transport classification fixed to the actual SDK discriminant (name: "NotFoundError") with tests; the owned-worker retry is now real (per-batch drain, capped at 3, 2-attempt give-up, batch-skip while the backlog is saturated).

Where we deliberately held your design: the quarantine mechanism (keying, cap, invalidation-on-update) is untouched — it's sound. The quiesce-retains-heartbeat sequencing is untouched — that was the right call. The TypeError-as-malformed classification stays, now documented as an explicit accepted tradeoff with its bounds.

Final state: 435 tests green, perf budgets verified under RECALL_PERF=1 (distill p50 0.85ms vs 150ms budget; rank p95 10.45ms vs 50ms), eval baseline intact. Merging once CI confirms on the updated head.

@rmk40
rmk40 merged commit a6d5b30 into rmk40:main Aug 28, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants