Skip to content

Commit 8c11845

Browse files
Branimir Rakiccursoragent
authored andcommitted
fix(network): honor opts.signal in agentDirectory adapter via race (Codex PR #496 round 5)
Codex review feedback: the previous revision dropped opts.signal in the only production AgentDirectoryLookup, leaving the resolver's documented cancellation guarantee unhonored. A slow SPARQL findAgentByPeerId could outlive the caller's deadline. DiscoveryClient itself doesn't (yet) accept an AbortSignal, but honoring the contract at the adapter boundary is enough: the adapter races the lookup against the abort signal, so when the signal fires the resolver gets an immediate `null` and can move on or surface a timeout. The underlying SPARQL fetch leaks (completes in the background and its result is discarded), bounded by the discovery client's own internal timeout. Acceptable trade-off because: - RFC 04 Phase 2 replaces this fallback path entirely - the alternative (full DiscoveryClient signal threading) is out of scope for this PR - the leak is bounded by an existing internal timeout, not unbounded The resolver pre-step `signal.aborted` check (added in round 3) still prevents NEW agent-directory work from starting once the deadline elapses; this round-5 race covers the case where the SPARQL is already in flight when the signal fires. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent b74de52 commit 8c11845

1 file changed

Lines changed: 33 additions & 14 deletions

File tree

packages/agent/src/dkg-agent.ts

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1326,20 +1326,39 @@ export class DKGAgent {
13261326
// when RFC 04 Phase 2 lands — at that point, the registry
13271327
// step takes precedence and this fallback is rarely hit.
13281328
//
1329-
// Note: AgentDirectoryLookup.findRelayForPeer accepts an
1330-
// optional `opts.signal` so the resolver can cancel an
1331-
// in-flight SPARQL on outer-deadline expiry. The legacy
1332-
// DiscoveryClient.findAgentByPeerId doesn't expose
1333-
// cancellation today (its SPARQL fetch is fire-and-await),
1334-
// so the signal is intentionally ignored here. Wiring it up
1335-
// is a follow-up that touches DiscoveryClient internals;
1336-
// until then the resolver's pre-step `signal.aborted` check
1337-
// still prevents NEW agent-directory work from starting once
1338-
// the deadline elapses, even if an already-running query
1339-
// can't be torn down.
1340-
findRelayForPeer: async (peerId, _opts) => {
1341-
const agent = await this.discovery.findAgentByPeerId(peerId);
1342-
return agent?.relayAddress ?? null;
1329+
// Codex review feedback on PR #496 round 5: the previous
1330+
// revision dropped `opts.signal` entirely, leaving the
1331+
// resolver's documented cancellation guarantee unhonored at
1332+
// the only production AgentDirectoryLookup. DiscoveryClient
1333+
// itself doesn't (yet) accept an AbortSignal, so we honor
1334+
// the contract at the adapter boundary instead: if the
1335+
// signal aborts the adapter resolves to `null` immediately,
1336+
// unblocking the resolver and the outer caller. The
1337+
// underlying SPARQL fetch then completes in the background
1338+
// and its result is discarded — a small leak in the abort
1339+
// path, acceptable given:
1340+
// (a) it's bounded by the discovery client's own internal
1341+
// timeout
1342+
// (b) RFC 04 Phase 2 replaces this fallback path entirely
1343+
// (c) the alternative (refactoring DiscoveryClient end-to-
1344+
// end signal threading) is out of scope for this PR
1345+
// The follow-up to plumb signals into DiscoveryClient is
1346+
// tracked separately.
1347+
findRelayForPeer: async (peerId, opts) => {
1348+
if (opts?.signal?.aborted) return null;
1349+
const lookup = this.discovery.findAgentByPeerId(peerId)
1350+
.then((agent) => agent?.relayAddress ?? null);
1351+
if (!opts?.signal) return lookup;
1352+
return Promise.race<string | null>([
1353+
lookup,
1354+
new Promise<null>((resolve) => {
1355+
opts.signal!.addEventListener(
1356+
'abort',
1357+
() => resolve(null),
1358+
{ once: true },
1359+
);
1360+
}),
1361+
]);
13431362
},
13441363
},
13451364
// Bootstrap is a libp2p-startup concern (`bootstrap({ list })` in

0 commit comments

Comments
 (0)