Skip to content

fix(coordination): release public routes with listener lifetime - #1291

Merged
drewstone merged 1 commit into
mainfrom
fix/coordinator-address-lifetime
Sep 18, 2026
Merged

drewstone merged 1 commit into
mainfrom
fix/coordinator-address-lifetime

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Change

Keep hosted, recursive coordinators from leaving stale public routes aimed at a local port after its listener closes. CoordinationPublicAddress.signal now ends on normal listener close or failed endpoint setup, as well as manager cancellation/deadline. The managed ingress can remove the route and abort its in-flight forwards before that port is reused.

Uses the existing callback and AbortSignal contract. No new scheduler, research policy, credential rule, dependency, public export, or durable format. The five existing empty Scope test doubles now supply the cancellation signal required by Scope rather than weakening the implementation.

Verification

Exact base d6104b386c04c7dfdadfd3a12735209ae3097247; patch release prepared as 0.241.1 through the maintained release script.

Read-only native verification run https://github.com/tangle-network/agent-runtime/actions/runs/35399078845 passed frozen install/peer checks, full tests, lint, source and example typechecks, verify:package, unchanged API surface, packed edge execution, documentation freshness and docs:check. Artifact includes the exact commit/patch, Git bundle, packed candidate and native logs. Separate delivery job handled verified Git objects only; no audit workflow or patch carrier enters this branch's tree or ancestry.

Two new real Scope/HTTP regressions failed on original source and pass with the repair: ordinary close while the manager stays live; and a failed public-address setup. The complete coordination file passes all 62 tests locally. The first full run exposed missing signals in old test doubles; those fixtures were corrected, not skipped.

This is one small dependency of sandbox-hosted Discovery. The companion work uses this lifetime to retire stable per-manager routes over one exposed ingress. This PR does not claim deployment, paid inference, indefinite uptime, cross-host fencing, or a live fleet-recovery proof.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

tangletools
tangletools previously approved these changes Sep 18, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — d3828903

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-18T22:01:18Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Value Audit did not run — no verdict

This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.

Status audit-incomplete (could not run)
Why value: agent returned no usable verdict; usefulness: agent returned no usable verdict
Lenses answered 0 of 2
What to do re-run once the CLI bridge has capacity: pr-reviewerctl trigger <repo>#<pr> --force

💰 Value — error

value agent produced no parseable value-audit JSON.

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 3
  • Bridge error: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1789768990299,"sessionID":"ses_f49748309ffesOd6GsOMi1ib33","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_98863eac"}}}; opencode/zai-coding-plan/glm-5.2: opencode: opencode error event without a message: {"type":"error",

🎯 Usefulness — error

usefulness agent produced no parseable value-audit JSON.

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 3
  • Bridge error: opencode/zai-coding-plan/glm-5.2: opencode: opencode error event without a message: {"type":"error","timestamp":1789769118799,"sessionID":"ses_f49748313ffekIhopZIDdD6BkM","error":{"name":"APIError","data":{"message":"Rate limit reached for requests","statusCode":429,"isRetryable":true,"responseHeaders":{"alt-svc":"h3=":443"; ma=3600","connection":"keep-alive","date":"Fri, 18 Sep 2026 22:05:18 GM

🔎 Heuristic Signals

🟡 Cruft: magic number added tests/kernel/coordination-mcp.test.ts

  •      perWorker: { maxIterations: 1, maxTokens: 1000 },
    

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260918T220611Z

@tangletools
tangletools dismissed their stale review September 18, 2026 22:06

Value audit could not run (value: agent returned no usable verdict; usefulness: agent returned no usable verdict). This approval was provisional on that audit running, so it is dismissed. Re-run the reviewer once the CLI bridge has capacity.

@tangletools

Copy link
Copy Markdown
Contributor

⚠️ Review Incomplete — d3828903

At least one required reviewer lane failed closed. No approval or request-changes review was published. This is a reviewer run failure, not a PR quality score.

Trigger a fresh review on the current PR head.

tangletools · 2026-09-18T22:21:42Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 3 (1 medium-concern, 1 low, 1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 136.2s (2 bridge agents)
Total 136.2s

💰 Value — sound-with-nits

Ties the exported public-address AbortSignal to the listener's actual lifetime (close, failed setup, or scope death) instead of only the manager's — a correct, in-grain contract fix whose only flaw is hand-rolling the abort cascade the codebase already ships as linkAbort.

  • What it does: serveCoordinationMcp now derives a listener-lifetime signal (coordination-mcp.ts:546-554) chained to opts.scope.signal and passes it to the publicUrl resolver as CoordinationPublicAddress.signal (coordination-mcp.ts:564) in place of the raw scope signal. The derived signal aborts when the scope aborts (reason forwarded), when endpoint setup fails (releaseAddress in the catch at :605), and on norma
  • Goals it achieves: A hosted/recursive coordinator that closes its listener mid-manager-lifetime no longer leaves a managed ingress route aimed at a freed local port (which the OS can rebind to another process). The resolver can now watch one signal for 'this address is dead' and remove the route plus abort in-flight forwards. This is the enabling lifetime for the companion sandbox-hosted Discovery ingress described
  • Assessment: Good change, coherent and in the grain of the codebase. It extends an existing callback+AbortSignal contract rather than adding machinery; all three exit paths (normal close, failed setup — including the preflight that runs inside the same try block — and scope abort/deadline) release the address; ordering is right (release before server.close so ingress forwards abort promptly). The two new tests
  • Better / existing approach: Partially — not for the design, but for the plumbing. The hand-rolled cascade at coordination-mcp.ts:546-554 (AbortController + addEventListener('abort', {once}) + already-aborted check + removeEventListener-then-abort release) exactly reproduces linkAbort from src/runtime/supervise/abortable.ts:81-103, whose header comment explicitly exists because 'written per layer, each drifts — a listener sto
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1789771699634,"sessionID":"ses_f494b2bb2ffe4pbVxaveu9xTUd","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_db188e87"}}}

🎯 Usefulness — sound-with-nits

A small, in-grain contract fix that ties the exported public-address AbortSignal to the listener's actual lifetime, reachable through the product supervise path today and consumed imminently by the hosted-ingress companion work; the only nit is that it hand-rolls the abort cascade the codebase alrea

  • Integration: Fully wired and reachable. The changed signal is the signal field of CoordinationPublicAddress (src/runtime/supervise/coordination-mcp.ts:119-126), passed to the caller-supplied publicUrl resolver at coordination-mcp.ts:564. serveCoordinationMcp is invoked by the real product path supervisorAgent (src/runtime/supervise/supervisor-agent.ts:930), which spreads the user's coordination opt
  • Fit with existing patterns: Fits the established pattern rather than competing with one: it refines the semantics of an already-exported AbortSignal instead of adding a new callback, event, or teardown hook, and the fixture/version churn is the repo's maintained release flow (scripts/prepare-release.mjs, scripts/generate-agent-improvement-proposal-fixtures.ts, check:testing-fixture). One grain mismatch: the hand-rolled `addr
  • Real-world viability: The lifetime correctly covers the realistic paths: normal close() while the scope stays live (releaseAddress at coordination-mcp.ts:637), failed setup including resolver throw, HTTPS refusal, and preflight failure (catch at coordination-mcp.ts:604-612), manager cancellation/deadline (abort listener at :548 with pre-aborted check at :549), and is idempotent under double-close (guarded at :552, ex
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: magic number added tests/kernel/coordination-mcp.test.ts

  •      perWorker: { maxIterations: 1, maxTokens: 1000 },
    

💰 Value Audit

🟠 Hand-rolled abort cascade duplicates the codebase's linkAbort helper [duplication] ``

coordination-mcp.ts:546-554 re-implements linkAbort (src/runtime/supervise/abortable.ts:81-103): already-aborted source check, once-listener, reason forwarding, and idempotent release-then-abort are all provided by that tested helper, which this file's sibling coordination-preflight.ts:26 already uses inside the same code path and whose module this file already imports (coordination-mcp.ts:49). abortable.ts's own documentation warns against exactly this per-layer hand-rolling. Replacing the bloc

🎯 Usefulness Audit

🟡 Hand-rolled abort cascade duplicates the codebase's linkAbort utility [problem-fit] ``

coordination-mcp.ts:546-554 hand-rolls a single-source abort cascade (controller + listener + idempotent release), but src/runtime/supervise/abortable.ts:81-103 provides linkAbort exactly for this — its doc says hand-written per-layer cascades 'drift' and it is 'the one cascade every executor uses' (interactive-worker.ts, bridge-executor.ts, runtime.ts, coordination-preflight.ts:26 — the last called from this same file). linkAbort(opts.scope.signal) yields the identical contract and addition


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260918T225034Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — d3828903

Review health 100/100 · Reviewer score 83/100 · Confidence 85/100 · 4 findings (4 low)

glm: Correctness 83 · Security 83 · Testing 83 · Architecture 83

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 5/5 planned shots over 9 changed files. Global verifier still owns final merge decision.

🟡 LOW Hand-rolled abort cascade duplicates linkAbort — src/runtime/supervise/coordination-mcp.ts

The addressLifetime controller plus addEventListener/aborted-check/releaseAddress reimplements exactly the cascade+release that src/runtime/supervise/abortable.ts:81 linkAbort provides (its own doc says hand-written per-layer versions drift). const address = linkAbort(opts.scope.signal) with releaseAddress = () => address.abort(new Error('coordination listener closed')) yields identical semantics — linkAbort.abort() releases listeners first and controller.abort is idempotent — in three lines with reason forwarding already tested. Cosmetic consistency nit; behavior is equivalent.

🟡 LOW Peer-mail listen failure after successful setup skips releaseAddress and leaves the main listener open — src/runtime/supervise/coordination-mcp.ts

The new contract on CoordinationPublicAddress.signal is 'aborts on close, failed setup, manager cancellation, or deadline', but await servePeerMail(mailbox, host) at line 615 runs outside the try/catch that guards the other setup failures. If it rejects (e.g. EMFILE, bind error), serveCoordinationMcp rejects without calling releaseAddress() and without closing the already-listening main server, so a resolver-provisioned public route never receives the abort and the port leaks until scope abort. The main-server leak is pre-existing, but the missing address abort is a gap in the newly documented lifetime. Fix: wrap the servePeerMail call (and mailb

🟡 LOW Address abort reason is not asserted, only the aborted boolean — tests/kernel/coordination-mcp.test.ts

Production distinguishes abort reasons: close/setup-failure abort with Error('coordination listener closed') (coordination-mcp.ts:553) while scope cancellation forwards opts.scope.signal.reason (coordination-mcp.ts:547). Both new tests only assert addressSignal?.aborted === true. Adding expect((addressSignal?.reason as Error)?.message).toBe('coordination listener closed') in the close test would pin the reason contract and catch a regression that silently forwards the wrong reason to endpoint provisioning. Impact is low: the boolean already catches the leak this PR fixes.

🟡 LOW Post-setup scope-cancel forwarding leg lacks a dedicated assertion — tests/kernel/coordination-mcp.test.ts

The new signal documents four abort triggers (close, failed setup, manager cancellation, deadline). Close and setup failure are covered by the new tests; manager cancellation is only exercised during resolution by the pre-existing pending-resolver test (line 1091), which aborts before setup completes. The forwarding listener path (scope aborts AFTER successful setup, coordination-mcp.ts:548) — the realistic 'manager dies while a public endpoint is provisioned' case — has no test asserting the registered address signal aborts. A small test cancelling the supervisor scope after serveCoordinationMcp resolves would close this gap. Low severity: the mechan


tangletools · 2026-09-18T22:55:00Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 4 non-blocking findings — d3828903

Full multi-shot audit completed 5/5 planned shots over 9 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-09-18T22:55:00Z · immutable trace

@drewstone
drewstone merged commit 90a3548 into main Sep 18, 2026
4 checks passed
@drewstone
drewstone deleted the fix/coordinator-address-lifetime branch September 18, 2026 23:26
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