Skip to content

fix(core): do not kill the process on a membership-watch delete timeout - #1066

Merged
davidfarah2003 merged 2 commits into
mainfrom
fix/1047-disarm-delete-timeout
Aug 30, 2026
Merged

fix(core): do not kill the process on a membership-watch delete timeout#1066
davidfarah2003 merged 2 commits into
mainfrom
fix/1047-disarm-delete-timeout

Conversation

@davidfarah2003

Copy link
Copy Markdown
Contributor

Closes #1047.

A live observer over a slow VPN died in disarmMembershipWatch when JS-API CONSUMER.DELETE timed out. That delete is cleanup. The broker reaps the consumer anyway. The timeout is not evidence the endpoint is unusable.

What changed

  • A delete timeout (and closed-connection) during disarm is caught, emitted as an endpoint error, and the disarm continues.
  • Fire-and-forget disarm from connection teardown also catches, so a leftover throw cannot become an unhandled rejection.
  • Non-timeout failures still throw. 404 still clears consumer identity.

Proof

  • Injected TimeoutError from consumer.delete on a simulated live open connection (the old policy swallowed timeout only when !nc or reconnecting; the crash had a live nc).
  • Smoke 5/5. Mutation A1 (restore live throw) and A2 (swallow without emit) KILLED on the named cells.

SHA: d4779dbe579b2849a276d36b645155a22b589e78

What this does NOT prove

  • Did not stall a real JS-API CONSUMER.DELETE over a TCP proxy or VPN. The fault is injected at consumer.delete(), which is the same rejection the NATS client raises. A live cotal web crash on the production mesh was not reproduced here.
  • Did not run smoke:delivery-reconnect:auth after retargeting its closed-epoch mutation find-string. That suite needs a broker and a core build.
  • ci-suites.txt is frozen until feat(cli)!: detach and persist meshes on up #880. This smoke is not appended.
  • Mutation-proof ran with --allow-dirty because this worktree has local node_modules copies.

CONSUMER.DELETE during disarm is cleanup. A timeout on a live observer
means the broker did not answer in time, not that the endpoint is
unusable. Catch it, emit an error, and continue.

@davidfarah2003 davidfarah2003 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LANDING-QUEUE REVIEW: APPROVE-quality

  • Diff matches body: adds a trailing .catch() on the membership-watch reset loop and changes disarmMembershipWatch (endpoint.ts ~2307-2320) to emit rather than throw on a timeout/closed-epoch during teardown, while still throwing for genuine non-timeout failures.
  • Evidence is real: disarm-delete-timeout.smoke.ts binds the actual disarmMembershipWatch off a live CotalEndpoint (5/5 cells matches the body's claim); mutation-proof A1/A2 target the exact new branch with named expectRed cells that line up with the smoke assertions.
  • bin/smoke/ci-suites.txt correctly left untouched per the body's stated reason; confirmed byte-identical to main's tail on this branch.
  • Interaction: test-merged this branch against current origin/main (which now carries the just-merged d2c0fd3/#1030 touching the same stop()-teardown block for presence/channel watch iterators) — auto-merges clean, no conflict, but the branch is 2 commits behind so a rebase before landing is still worth doing.
  • Body's disclosed evidence gaps (no real TCP/VPN CONSUMER.DELETE stall, smoke:delivery-reconnect:auth not re-run after retarget, mutation-proof run with --allow-dirty) are honestly named, not hidden.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

BLOCK at exact head d4779dbe579b2849a276d36b645155a22b589e78.

The behavior repair itself is supported:

  • pnpm smoke:disarm-delete-timeout: 6 cells green. A live delete timeout resolves, emits one endpoint error, and keeps consumer identity; non-timeout failures still throw; 404 clears identity.
  • Mutation proof: both new mutations were killed red-and-named, covering the old live throw and a silent swallow without error emission.
  • pnpm --filter @cotal-ai/core build && pnpm smoke:delivery-reconnect:auth: green on the real-broker membership watch reconnect, stop, retained consumer, and cleanup paths.
  • Core typecheck and diff check are clean.

Blocking finding: pnpm smoke:mutation-fixtures fails. The retargeted existing mutation named closed epoch delete rejects public stop instead of deferring cleanup in packages/core/smoke/mutations/membership-feed-reconnect.json now uses a find window that contains three source comment lines. The fixture guard reports ANCHOR SPANS A COMMENT and refuses it. This is load-bearing: a comment-only edit could silently disarm the mutation without changing behavior.

Re-anchor that mutation on a unique code-only window and rerun both smoke:mutation-fixtures and its mutation proof. The behavior fix does not need redesign; the regression guard needs to be made durable.

I rechecked refs/pull/1066/head immediately before this verdict and it still matched the SHA above.

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

Delta review at exact head 29bfaae08b5020c1993adc4916606a1e158fdf74 (re-read gh pr view headRefOid immediately before this comment; matches git rev-parse HEAD in a detached worktree of that object). Parent of the delta: d4779dbe579b2849a276d36b645155a22b589e78 is an ancestor. Load 33.15, 29.21, 28.23. Did not run cotal up. This is a bounded delta review of the mutation re-anchor, not a re-grade of the behaviour repair.

Verdict: APPROVE (delta only)

The claimed delta is true:

  • git diff --name-only d4779dbe 29bfaae0packages/core/smoke/mutations/membership-feed-reconnect.json only.
  • git diff --stat1 file changed, 1 insertion(+), 1 deletion(-).
  • Fast-forward: one commit 29bfaae0 test(core): re-anchor membership cleanup mutation.

That one find-string dropped the comment-spanning window. The replacement is:

if (timeout || closedEpoch || dyingEpochTimeout) {
  this.emit("error", err as Error);
} else {
  throw err;
}

No // or /* inside it. Count in packages/core/src/endpoint.ts at this head: 1 (unique). The old find included the prose "Throwing here killed a live observer over a slow VPN (#1047)."

Mutation still KILLS

node scripts/mutation-proof.mjs --config packages/core/smoke/mutations/membership-feed-reconnect.json at this head: 5/5 KILLED. The re-anchored mutation ("closed epoch delete rejects public stop instead of deferring cleanup") is red and named on public stop concurrent with terminal close resolves after fresh cleanup at 22 marks vs baseline 23. Uniqueness without a kill would have been a BLOCK; this is both unique and killing.

Fixture sweep

pnpm smoke:mutation-fixtures at this head: 223 files, 1225 anchors. dead 0, ambiguous 0, spanning prose 0, missing 0. Sweep does not prove any of them still kill; the reconnect fixture's kill is the run above.

Behavioural half of the original BLOCK, re-checked at the new head

pnpm smoke:disarm-delete-timeout: 6 cells green (DISARM-DELETE-TIMEOUT SMOKE OK). Live delete timeout does not reject disarmMembershipWatch; timeout is one endpoint error; consumer identity kept; non-timeout still throws; 404 still clears.

Not proven / not in scope

  • Full-repo typecheck/build / pnpm check / smoke:ci.
  • Whether this head is current with origin/main 41502977 (behind is not a finding for this delta).
  • Re-litigating the process-kill repair itself beyond the disarm suite still being green.

No other delta-review comment named 29bfaae0 when I started.

@davidfarah2003
davidfarah2003 merged commit 98ebcfa into main Aug 30, 2026
2 checks 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.

core: an unhandled consumer-delete timeout in disarmMembershipWatch kills the whole observer process

1 participant