2778: Hosted Factory: run the discover→triage→dispatch loop in Cloud#126
Conversation
kjgbot
left a comment
There was a problem hiding this comment.
Review — Hosted Factory control-plane loop (@agent-relay/factory/hosted)
Reviewed by ar-2778-review-cloud at head 18689a7. This is a strong, well-architected drop: a genuinely worker-safe engine (no Node built-ins — the esbuild browser-target test proves it), clean port boundaries, deterministic invocation IDs, stable writeback idempotency keys, and a DO-backed transactional store. Validation reproduced locally: npm run build ✓, full suite 972/972 ✓, hosted suite 12/12 ✓, npm pack includes the /hosted dist ✓. The tsc --noEmit errors are all pre-existing in untouched test files (excluded from tsconfig.build.json), not introduced here.
One required change before this ships as the safety primitive it's meant to be, plus two minor recommendations.
🔴 Required — claimRunLease can grant two live holders the same epoch (split-brain window)
src/hosted/state-store.ts:38-46 (InMemory) and :156-164 (DurableObject) — the continuingLiveClaim branch.
const continuingLiveClaim = current?.ownerId === ownerId && current.expiresAtMs > nowMs
const lease = { ..., epoch: continuingLiveClaim ? current.epoch : (current?.epoch ?? 0) + 1, ... }
return { acquired: true, lease }A fresh claim returns acquired:true reusing the same epoch whenever it arrives with an ownerId string equal to a still-live lease. Both claimants then pass matchesLiveLease in every subsequent renewRunLease/saveIssue, so both write — the exact split-brain the DO lease exists to prevent (issue gap #6).
Why this is reachable and not just a contract nicety:
HostedFactoryLoop.#operationSequenceis instance state initialized to0(orchestrator.ts:37), so the per-op suffix${ownerId}:${operation}:${seq}(orchestrator.ts:384-390) resets to 1 on every fresh loop instance. Two loop instances constructed per invocation (the exact pattern in the README example) that share a baseownerIdboth compute…:sweep:1— identical owner strings.- The
ownerIddoc comment intypes.ts:155says "Each operation adds its own sequence fence." Across instances that is false — the seq provides no cross-instance protection, yet the comment invites a reader to reuse a stable baseownerId(e.g. a DO-scoped id) believing the suffix protects them.
Concrete failure (two isolates, same ownerId, DO store): X claims epoch 1 → triages → saves dispatching with invocations [A,B] → spawns A,B. Y (stale snapshot, continuingLiveClaim=true, epoch 1 reused, acquired:true) saves triaging with invocations []; its transactional saveIssue sees prior=[A,B] and deletes the invocation index for A and B (state-store.ts:220-224). The completion for A then hits findIssueByInvocation → null → ingestCompletion → not-found; A's work is orphaned and the issue can hang. (The invocation-ID / idempotency-key design still prevents duplicate spawns/writebacks — but not this state clobber.)
Note this branch is dead code for correct single-instance use: within one loop instance the seq always increments, so overlapping ops get distinct owner strings and fence each other via the ownerId !== ownerId guard at :35. So continuingLiveClaim is only ever exercised by the collision case above.
Fix: make a fresh claim always bump the epoch (drop the continuingLiveClaim reuse). Renew — not re-claim — is the live-lease keepalive, so always-increment still lets a crashed host's fast restart resume, while fencing any prior in-flight holder of the old epoch on its next save. Strictly safer, no downside. Please also correct the types.ts:155 comment, and consider deriving a per-instance random ownerId salt and/or a runtime uniqueness guard so the guarantee doesn't rest on caller discipline. Worth adding a same-owner live-reclaim test (the existing state-store.test.ts:99-102 only reclaims after expiry, so this branch is untested).
🟡 Recommended — undispatchable triage decision is a silent poison pill
src/hosted/orchestrator.ts:150-153 — if (record.invocations.length === 0) throw fires before the #save, so the record stays in triaging and every subsequent runOnce re-triages and throws again forever, with no clarification/dispatch writeback ever posted (a human is never notified). Not reachable with the shipped TieredTriage (empty routes force confidence:'low' → awaiting-clarification first), but triage is a pluggable port — a custom engine returning scope:'workflow', workflow:undefined, confidence:'high' triggers it. Prefer routing an empty-spec decision to awaiting-clarification/blocked with a reason over an uncaught, un-persisted throw.
🟡 Minor — releaseRunLease in finally can mask the run result
orchestrator.ts:175 and :216 — if releaseRunLease rejects (DO storage error), it throws out of the finally and masks the computed report/result. A try/catch around the release keeps the real outcome.
Everything else I probed is correct: epoch monotonicity across the release tombstone, stale-write rejection after takeover, at-least-once terminal-wins in applyCompletion, partial-dispatch resume, merge-gate re-poll, the DO invocation-index rewrite, and HostedFactoryLeaseLostError propagation. Happy to re-review promptly once the fencing change lands. Not merging per policy — leaving this for human approval.
kjgbot
left a comment
There was a problem hiding this comment.
All requested lease-fencing changes are addressed at dad49dd: every fresh claim increments the epoch (including a same-owner live reclaim), empty custom triage plans persist and request clarification, and lease-release failures no longer mask results. Added regression coverage; full suite passes (976 tests) and the production build passes.
Context
Factory today requires a control plane you run yourself. The supported topology is one Factory control-plane host per workspace plus any number of execution nodes; active/active control planes are explicitly unsupported because separate local state files can't provide a truthful cross-host fence.
That means the current story is:
factory start --mode liveon a machine you own, optionally with--backend relayto place agents on hosted fleet nodes, and Cloud for observability. There is no way to get a Factory without operating a host.Running the loop in Cloud is the logical end state — the same shape as one-click agent deploys.
What exists today
A partial cloud-side arm is already on
main, dormant:packages/web/lib/proactive-runtime/factory-cloud-orchestrator.ts—dispatchFactoryBrainDelivery()(:280),isFactoryBrainEnabled()(:154)factory-fleet-emitter.ts— spawns via Relaycast actionsfactory-state-store-do.ts/factory-state-store-core.ts— DO-backed in-flight stateintegration-watch-dispatcher.ts(:2075-2265) — Linear[factory]matching armLanded in #2226, #2227, #2231, #2234. It is webhook-driven dispatch only, not the full loop.
Gaps between that and a hosted Factory
ingestFactoryInvocationCompletion()(factory-cloud-orchestrator.ts:162) has no production caller — only tests. With the flag on, spawns would be emitted and never resolved: no merge gate, no Linear/Slack writeback.isFactoryBrainEnabled()reads SST resourceCloudFactoryBrainEnabled→CLOUD_FACTORY_BRAIN_ENABLED→FACTORY_BRAIN_TEST_MODE. The comment at :105-110 claims the SST resource "is registered and default-seeded to false", butCloudFactoryBrainEnabledappears nowhere ininfra/secrets.ts, the seed scripts, orlib/boot/resource-check.ts— unlike e.g.CloudTeamLaunchN1Enabled. Only the env path works, so it is effectively test-only. That stale comment is worth fixing regardless of this issue, since it misrepresents what is deployable./dashboard/factoryis read-only (#2762, #2764, #2770). There is no enrollment, config, or start surface.Suggested shape
Probably staged rather than one drop:
Related
Fixes #2778
Summary by cubic
Runs the full hosted Factory control-plane loop in Cloud, covering discovery, triage, dispatch, completion reconciliation, merge gate, and provider writeback. Adds a worker-safe
@agent-relay/factory/hostedentrypoint and Durable Object–backed fencing with reclaimed-lease protection to safely support active/active in Cloud. Fixes #2778.@agent-relay/factory/hosted:createHostedFactory,InMemoryHostedFactoryStateStore,DurableObjectHostedFactoryStateStore, and types.hostedFactoryInvocationIdand idempotent provider writebacks; safe retry when a spawn acknowledgement is lost.package.json./hostedexport, README usage, worker-safe bundle check, dist entrypoint import test, and orchestrator/state-store tests.Written for commit dad49dd. Summary will update on new commits.