refactor(daemon): run the orphan reconciler as a one-shot CronJob - #1079
Conversation
Move the pool's orphan sweep out of every `--k8s` member and into `agentconnect-daemon reconcile --once`, a subcommand that boots only the sandbox API surface and a control-plane connection, runs one sweep, prints the summary line, and exits 0 (non-zero when the sweep could not complete). The cluster owns what the in-process version built by hand: the schedule, the mutual exclusion (`concurrencyPolicy: Forbid` in place of a single-holder lease), and the failure reporting a Job status already gives an operator. The job's connection registers as an OBSERVER: a new optional `register.observer` flag the control plane admits on exactly the member TokenReview path, then withdraws from the member set `upsertOnAuth` enrolled it in and backdates so the pool-member reaper retires the row promptly. Duty eligibility is a membership lookup, so a daemon in no set can never be granted work; the flag is refused with SCOPE_DENIED on an org-scoped connection. `agent/exists` serves it unchanged. Removed with the in-process path: the scheduler and its jitter, `AC_K8S_ORPHAN_SWEEP_INTERVAL_MS`, the `sweep_leases` table and `LocalStore.acquireSweepLease` (no migration step — the table only ever lived in the CREATE block for one prerelease), the daemon-side `agent-exists-v1` feature gate, and `CpClient.agentsExist`. The grace becomes the object's own age alone: a one-shot run carries no memory of an earlier sweep, and object age is the clock that guards the race the grace exists for. Refs #1062
There was a problem hiding this comment.
I found one blocking reliability issue in the new one-shot Job contract.
runReconcileOnce currently exits successfully whenever OrphanReconciler.sweep() returns a summary. Individual Kubernetes delete errors are caught inside the sweep and recorded in summary.failed, so a run that fails to remove one or every orphan still exits 0. Kubernetes therefore records a successful Job and the stated failure-reporting/alerting behavior is lost. Please make any nonzero failed count produce a nonzero process exit.
I verified the exact trusted base/head revision and inspected the complete daemon, protocol, control-plane, documentation, and test diff. Static whitespace/conflict-marker checks passed. I could not execute the Vitest/typecheck suites because this checkout has no installed dependencies.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| if (!url) throw new Error(`reconcile requires the control plane's address in ${CP_URL_ENV}`) | ||
| cp = await (opts.connectCp ?? connectObserver)(url) | ||
| const reconciler = new OrphanReconciler({ api, liveAgents: cp.liveAgents, settings, log }) | ||
| return (await reconciler.sweep()) ? 0 : 1 |
There was a problem hiding this comment.
[P2] Return nonzero when object deletion failed
sweep() returns a summary even when one or more Kubernetes deletes failed; those failures are recorded in summary.failed. Since every summary object is truthy here, a run that fails to delete all candidates still exits 0 and Kubernetes records a successful Job, defeating the new failure-reporting contract. Please return nonzero when summary.failed > 0 (while continuing to treat undefined as failure).
A delete that fails is counted in the sweep summary rather than thrown, so the run still reports everything it found in one line — but returning 0 on that summary recorded a successful Job for a run that left an orphan behind, and the leak would simply be back on the next run with nothing in the cluster's history saying so. Exit non-zero when `summary.failed > 0`, after the summary line the sweep already logged; an unreadable sweep stays a failure exactly as before.
|
Good catch — fixed in 22d5c90. A failed delete is counted in Test added: a sweep whose delete is refused 403 still logs Re-ran |
There was a problem hiding this comment.
The prior blocking finding is resolved in 22d5c90513ecf5e35cf7b05bea0b228e82ba3cdc: the one-shot reconciler now returns a nonzero exit when its completed sweep reports any failed Kubernetes deletion, while an unreadable sweep remains a failure. The added regression test covers the partial-delete failure and confirms the full summary is still logged before exit.
I verified the exact trusted head, confirmed the fix delta is limited to the exit-status correction and its test, and rechecked the complete base-to-head daemon/protocol/control-plane behavior. Static whitespace and conflict-marker checks passed. I could not execute Vitest or typecheck because this checkout has no installed dependencies. No blocking findings remain.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
|
Confirmed the fix in sent by |
Summary
The pool's orphan reconciler (#1074) moves out of the daemon process and becomes a one-shot
subcommand a Kubernetes CronJob runs:
agentconnect-daemon reconcile --once. It boots only what asweep needs — the sandbox API surface and a control-plane connection for the
agent/existsread —runs exactly one sweep, prints the summary line, and exits 0 (non-zero when the sweep could not
complete). The in-process timer, the jitter, the interval knob, and the
sweep_leasestable aregone.
The connection the job opens registers as an observer, a new optional
register.observerflag,so a process whose only job is to sweep can never be handed work to serve.
Why a CronJob
Everything the in-process sweep built by hand is something the cluster already owns:
setTimeouton every member.concurrencyPolicy: Forbidinstead of a single-holder lease in the sharedstore. One run at a time, enforced by the thing that starts the runs.
control plane now exits non-zero instead of logging a warning inside a daemon that keeps running.
turns bought nothing and cost a scheduler, a lease table, and a feature gate.
One behavioural consequence, stated deliberately: the grace period is now the object's own age
alone. The old code also required the agent to have been missing across the sweeping member's own
sweeps, and a one-shot run has no memory of an earlier run to carry that in. The object-age gate is
the clock that actually guards the race the grace exists for — an in-flight creation still racing
the control plane's write — and it survives; the reconciler also still ships dry-run by default.
Observer registration
RegisterReqgains an optionalobserver: true. On an install-wide (pool identity) connection thecontrol plane admits it exactly as it admits a member — same TokenReview path, same projected
ac-control-planetoken, sameauth→registerhandshake — and then:upsertOnAuthmints for every org-less row. Duty eligibility is amember_set_memberlookup (claimVacant'seligibleAgentgate), so a daemon in no set can neverbe granted a set-placed group, and a machine-placed agent never names it.
lastSeenAt, so the existing pool-member reaper retires the row on its next sweeprather than after the ordinary 15-minute silence window. Marking the row is the smaller change:
skipping the row entirely is not available, because the epoch mint, the connection registry, and
every reply are keyed by a daemon id that has to exist. An observer sends no heartbeat, so nothing
moves the stamp forward again.
The flag is refused on an org-scoped (API-key) connection with
SCOPE_DENIED: that credentialis an operator's daemon key, not a job identity, and there is no reason to let it opt out of
membership.
agent/existsalready served install-wide connections and now serves observers unchanged.What was removed
OrphanReconciler's scheduler:start()/stop(), the ±25% jitter,AC_K8S_ORPHAN_SWEEP_INTERVAL_MS,and the in-flight single-flight guard.
AC_K8S_ORPHAN_DELETE(default off = dry-run) andAC_K8S_ORPHAN_GRACE_MSare unchanged.sweep_leasestable andLocalStore.acquireSweepLease. No migration step: the table onlyever existed in the
CREATE TABLE IF NOT EXISTSblock, was introduced in a single prerelease, andis never read again — an existing store simply keeps an unused table until it is recreated.
agent-exists-v1"). It only existed to keep an in-process sweep quiet against an older controlplane; the job now fails loudly instead, which is what a Job status is for. The control plane
still advertises the feature.
CpClient.agentsExistand the daemon'sliveAgentsFor, with the reconciler's wiring inDaemon/startK8sRuntimePlane. Members run no sweep at all now.The CronJob the deployment side should add
Image = the daemon image, args
reconcile --once, every 10 minutes, the pool members'ServiceAccount, the same namespace/warm-pool/labels environment they read, and
AC_K8S_ORPHAN_DELETEleft unset so it starts dry-run:The command needs
AC_CP_URLandAC_K8S_SANDBOX_NAMESPACE, plus the projected identity token at/var/run/ac-cp-identity/token. Its RBAC is the members' existing sandbox-namespace Role (list anddelete
sandboxclaims, list and deletesandboxes); a Role without the Sandbox list verb simplynarrows the sweep to claims, as before.
Test plan
packages/daemon/test/cli-reconcile.test.ts(new) — the subcommand against the fake API serverplus a fake control plane: one sweep, one existence read, the summary line, exit 0; exit 1 when
the control plane cannot be reached (deleting nothing) and when the sandbox namespace is unset.
Plus the observer handshake itself over a fake transport:
auththenregisterwithobserver: true, theagent/existschunking, and a refused registration failing the connection.packages/control-plane/test/protocol/observer-register.handler.test.ts(new, real Postgres) —an observer register leaves no
member_set_memberrow and is granted nothing from a vacant dutygroup over a set-placed agent, while an ordinary member of the same pool claims that same group;
agent/existsis answered on the observer connection; the row is selected byfindRetiredPoolMembersimmediately; the flag is refused withSCOPE_DENIEDon an API-keyconnection.
packages/daemon/test/k8s-orphan-reconciler.test.ts— the feat(daemon): pool orphan reconciler for leaked sandbox objects #1074 safety rules kept, minus thelease and scheduler cases, with the grace now asserted on the object's own age.
pnpm lint,pnpm format:check, typecheck for protocol / daemon / control-plane, the daemontest/cp,test/k8s-*,test/local-store.test.tsandtest/cli-reconcile.test.tssuites, and thecontrol-plane unit + integration projects.