[Experiment] orchestrator-v2 hardening: telemetry, resilience, capability routing#42
Conversation
- Route A: propagate execution telemetry from the engine through the runner RPC to the TaskSource as a terminal outcome; unify ExecutionStats in interfaces-task-source. - G1: persist sessionId (and telemetry) on the failure path so a failed run stays resumable. - I3: guard task-source callbacks so a throw can't leak a peer's in-flight slot, hang the runner's terminal RPC, or escape as an unhandled rejection (settle/recordBestEffort helpers + a route().catch net). - I1: capability-aware routing -- runners advertise their registered agents in the heartbeat; the orchestrator only dispatches a task to a capable peer (backward compatible: a runner that advertises nothing = capable of anything). - Adds regression tests (orchestrator.spec I3, peer-registry.spec I1). 43/43 tests pass; build + lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- examples/claude-engine: a runnable real-engine deployment (runner.yaml onboarding + a claude-CLI Engine that maps CLI JSON to EngineResult, so real token/cost telemetry flows). Registers examples/* as a workspace member. - docs/experiment: the session's gap report, retrospective (lessons), and dogfood-run notes with telemetry receipts (three escalating real-engine runs, incl. the G1 resume payoff). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| ScriptTaskDefinition, | ||
| } from "./types.js"; | ||
| export { isScriptTaskDefinition } from "./types.js"; | ||
| export type { ExecutionStats } from "@bifrost-ai/interfaces-task-source"; |
There was a problem hiding this comment.
I don't think execution stats belong to the task source. They're a core part of the orchestrator and can exist for non task-source'd things
| selected: ConnectedPeer | undefined; | ||
| }; | ||
|
|
||
| const fakePeer = (peerId: string): ConnectedPeer => ({ |
There was a problem hiding this comment.
Context and helpers should be below describe blocks. 30 lines of non-tests to get to the real tests is a lot to skip
| } | ||
| state.runnerId = payload.runnerId; | ||
| state.lastSeen = Date.now(); | ||
| if (payload.capabilities !== undefined) { |
There was a problem hiding this comment.
So we send the capabilities every time, but only update them once? What if an agent updates their capabilities dynamically?
| function canHandle(state: PeerState, requiredCapability: string | undefined): boolean { | ||
| // No requirement, or a runner that hasn't advertised its capabilities, is treated as | ||
| // able to handle the task (keeps non-advertising runners working as before). | ||
| if (requiredCapability === undefined || state.capabilities === null) { |
There was a problem hiding this comment.
I like the capabilities. I think we should move forward with the expectation that all runners will advertise and treat null as no capabilities
| return { method: record.method, args: record.args }; | ||
| } | ||
|
|
||
| function errorMessage(error: unknown): string { |
There was a problem hiding this comment.
This should be part of send rpc error
| ): HeartbeatHandle { | ||
| const send = () => { | ||
| peer.send({ kind: "heartbeat", runnerId: identity.keyId }); | ||
| peer.send({ kind: "heartbeat", runnerId: identity.keyId, capabilities: [...capabilities] }); |
There was a problem hiding this comment.
Don't need to clone the array. Pass it directly and ensure peer send doesn't modify parameters
|
Superseded by #43 — this PR got auto-closed when I deleted the source fork (GitHub closes a cross-fork PR once its head repo is gone). #43 re-opens the same two commits on a branch in the main repo, so it won't be fragile like this one. Carrying your review feedback over to #43 — thanks for the thorough pass! 🙏 |
2 participants
Summary
Experiment / exploratory (opened as a Draft): hardens the
orchestrator-v2rebuild —telemetry propagation, failure resilience, and capability-aware routing — plus a runnable
real-engine example and a full write-up. Not intended to merge as-is; see
orchestrator-v2/docs/experiment/.Motivation
A session that audited and dogfooded orchestrator-v2 found that its "thin" terminal boundary
dropped everything but
taskId— execution telemetry, task-source callback errors, and runnercapability all had to be threaded deliberately. This branch does that threading, with regression
tests, and captures the findings.
Type of change
/simplifypassesorchestrator.spec(I3),peer-registry.spec(I1)Checklist
go/npxwere not usedmake lintpasses — N/A:orchestrator-v2uses its ownvp/oxlint toolchain (not wired intomake);vp checkis cleanmake testpasses — N/A:vp run -r testis green (43/43)make buildpasses — N/A:vp run -r buildis cleanmain; branch is current withmain/simplifypasses)Notes for reviewers
pursued, it should split into focused PRs (Route A / I1 / I3).
shape as the telemetry gap, for the result), reconcile 3 correctness-level doc gaps, build
agent-4-workflow+ aTaskSourceread-side.canHandle's fail-open default (nullcapabilities → handles anything) as surprising. It's deliberate (backward-compat for
non-advertising runners, load-bearing for the stub-runner tests) but deserves an explicit
rationale comment or an opt-in.
orchestrator/package.json(thetest-helpersexport) was deliberately leftout of this branch.
CONTRIBUTING.mdpredatesorchestrator-v2/and doesn't document itsvptoolchain — worth updating.🤖 Generated with Claude Code