feat(meta): DO-based role arbiter replacing Neon cc_node_leases - #131
Merged
Conversation
added 2 commits
August 3, 2026 02:06
ADR-001 elected a meta-orchestrator leader across chittymini-01..06 using
Neon `cc_node_leases`. Two problems with that in practice:
- The daemon runs nowhere. No systemd unit, no daemon/loop process on
chittyserv-vm. The Worker plane being up masks it: the interface is
live while the always-on coordinator is not.
- The fleet it floats across is 6/7 offline (chittymini-01..06 last seen
~7d). chittymini-00 is the operator seat and must not hold persistent
infra, so a leader designed to float has nowhere to float.
A Durable Object is already a strongly-consistent, single-threaded
singleton. Using one does not reimplement leader election — it removes the
need for it, along with the fleet dependency and the Neon lease table.
Neon cost pressure makes this favourable; it would be the right shape
regardless.
CommandCoordinator (meta/coordinator.ts) arbitrates role leases in DO
storage. daemon/coordinator-lease.ts is a drop-in client exporting the same
four functions with identical signatures, so daemon/loop.ts switches by
changing one import. It fails closed with
POLICY_BLOCKED_COORDINATOR_UNAVAILABLE rather than making an unarbitrated
local decision, which would permit split-brain.
Wire semantics are a deliberate 1:1 port of the SQL, preserving both prior
review findings: session ownership required on heartbeat (codex-p2 PR#101
finding-5) and on release (finding-2).
Nodes keep a role as executors that pull work — justified by needing local
filesystem and repo access. Leader election never was that reason.
Tests: 13 cases in real workerd against real DO storage, no mocks. Covers
exclusion, idempotent re-claim, expiry takeover, both session-ownership
rejections, heartbeat extension, release/reclaim, role isolation, lease
clamping, and validation.
Scope: daemon/leader.ts and the Tier-5 Neon usage are untouched. Migrating
those is a separate decision that should be made on spend data.
Separated review (silent-failure-hunter, fresh context) found three real
defects plus a vacuous test. Split-brain, SQL semantic parity, and auth
came back clean and are unchanged.
P1 — daemon/coordinator-lease.ts could not load on Node. It imported
META_LEADER_ROLE (a value) from meta/coordinator.ts, which evaluates
`cloudflare:workers`. The documented one-line switch in daemon/loop.ts
would have crashed the daemon at module load, before main() and before any
log line: no leader claimed, intent queue silently stopped. tsc passed
because the failure is runtime/bundle-only. Extracted the runtime-free
meta/lease-types.ts; both sides import from it.
Verified: `esbuild --platform=node` fails on the old code with
`Could not resolve "cloudflare:workers"` and succeeds on the new.
P2 — fail-open on partial config. COORDINATOR_URL set with no
COORDINATOR_TOKEN sent unauthenticated requests, drawing a 401 that
loop.ts logs as a claim error and retries forever — a permanently dead
daemon whose logs read like a transient auth blip. Both values are now
required, failing closed with POLICY_BLOCKED_COORDINATOR_UNAVAILABLE.
P3 — greedy path regex. /^.*\/coordinator/ matched the LAST occurrence, so
`/api/meta/coordinator/a/coordinator/release` dispatched `release` from a
path that does not name it. Anchored on the first segment.
Also: a corrupted leaseExpiresAt parsed to NaN, and every NaN comparison is
false, leaving the role permanently unclaimable — a fail-closed deadlock
with no SQL analogue, since Postgres typed the column. Unparseable now
reads as expired.
Tests 13 → 24. The review's sharpest point was that the suite covered the
class and skipped the seam where P1 lived, so this adds a full HTTP surface
suite: path parsing, method switch, 404s, malformed-body 400, the bare-null
describe signal, and the {released} envelope the client unwraps. Plus
MIN-clamp, release-by-other-node, omitted-vs-null sessionId, and corrupt-
expiry recovery.
`extends the expiry on a valid heartbeat` was vacuous — it claimed at 1s and
heartbeat at 60s, so the assertion held by construction and would have
passed against an implementation computing expiry from claimedAt. Rewritten
to use identical leaseSeconds on both calls.
Both new regression tests were mutation-checked: reverting each fix makes
exactly that test fail.
Kept on review advice: claimedAt preserved across takeover. Not a defect —
it reads as "when this role was first continuously held" — and it is
load-bearing, since the client returns null on falsy claimedAt. Comment
corrected to say so.
Not fixed here (pre-existing, outside this diff): daemon/loop.ts:277-295
logs exec_heartbeat_lost on takeover without aborting the in-flight
dispatch, so a demoted leader finishes its current intent alongside the new
one. That is the one real split-brain path and it is unreachable from the
DO.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
chittycommand | b71ac24 | Aug 03 2026, 02:18 AM |
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughChangesCoordinator-backed lease management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LeaseClient
participant WorkerRoute
participant CommandCoordinator
participant DurableObjectStorage
LeaseClient->>WorkerRoute: Send authenticated lease operation
WorkerRoute->>CommandCoordinator: Forward request to meta-orchestrator
CommandCoordinator->>DurableObjectStorage: Read or update stored lease
DurableObjectStorage-->>CommandCoordinator: Return lease state
CommandCoordinator-->>WorkerRoute: Return JSON result
WorkerRoute-->>LeaseClient: Return coordinator response
Possibly related PRs
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
To use Codex here, create a Codex account and connect to github. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
ADR-001 elects a meta-orchestrator leader across
chittymini-01..06using Neoncc_node_leases. Two things are true on the live system today:chittycommandsystemd unit and nodaemon/loopprocess onchittyserv-vm. The Worker plane being up masks it — the interface is live while the always-on coordinator is not, so "brain never stalls" is currently false.chittymini-01..06last seen ~7 days ago.chittymini-00is the operator seat and must not hold persistent infra. A leader designed to float freely has nowhere to float.A Durable Object is already a strongly-consistent, single-threaded singleton. Using one does not reimplement leader election — it removes the need for it, along with the fleet dependency and the Neon lease table. Neon cost pressure makes this favourable; it would be the right shape regardless.
What
meta/coordinator.ts—CommandCoordinatorDO arbitrating role leases in DO storagemeta/lease-types.ts— runtime-free vocabulary shared by workerd and Nodedaemon/coordinator-lease.ts— drop-in client, same four exports and signatures asdaemon/leader.ts, sodaemon/loop.tsswitches by changing one import/api/meta/coordinator/*, behind the existingauthMiddlewareFails closed with
POLICY_BLOCKED_COORDINATOR_UNAVAILABLErather than making an unarbitrated local decision, which would permit split-brain. Wire semantics are a 1:1 port of the SQL, preserving both prior findings: session ownership on heartbeat (codex-p2 PR#101 finding-5) and on release (finding-2).Nodes keep a role as executors that pull work — justified by needing local filesystem and repo access. Leader election never was that reason.
Validation
npx tsc --noEmitcleanesbuild --platform=nodebundles the daemon client clean (regression check — see below)Separated adversarial review
Reviewed by a different agent in a fresh context, prompted to break it. Split-brain, SQL semantic parity, and auth came back clean (concurrent-claim trials through the real HTTP path, clause-by-clause SQL comparison, 401/200 auth confirmation). Three real defects found and fixed in b71ac24:
meta/coordinator.ts, pullingcloudflare:workersinto a Node process. The documented one-line switch would crash the daemon at module load — beforemain(), before any log line.tscpassed; failure is runtime/bundle-only.meta/lease-types.tsCOORDINATOR_URLset withoutCOORDINATOR_TOKENsent unauthenticated requests → permanent 401 retry loop that logs like a transient blip/^.*\/coordinator/matched the last occurrence, so/a/coordinator/releasedispatchedreleasePlus a corrupt
leaseExpiresAtparsing toNaNleft the role permanently unclaimable (no SQL analogue — Postgres typed the column).The review's sharpest finding was that my original 13 tests covered the class and skipped exactly the seam where P1 lived. Tests went 13 → 24, adding the full HTTP surface. One test (
extends the expiry on a valid heartbeat) was vacuous — it claimed at 1s and heartbeat at 60s, so the assertion held by construction; rewritten.Both new regression tests were mutation-checked: reverting each fix makes exactly that test fail.
Scope
daemon/leader.tsand all Tier-5 Neon usage are untouched.daemon/loop.tsstill imports./leader— this PR makes the switch possible, it does not make it. Migrating the Tier-5 app off Neon (~20 files,postgresql→sqlite) is a separate decision that should be made on spend data.Known, not fixed here
daemon/loop.ts:277-295logsexec_heartbeat_loston takeover without aborting the in-flight dispatch, so a demoted leader finishes its current intent alongside the new one. Pre-existing, unchanged by this PR, and unreachable from the DO — but it is the one real split-brain path.🤖 Generated with Claude Code
https://claude.ai/code/session_01Cz2Px2ekaArQVnCjz3RT1x
Summary by CodeRabbit
New Features
Bug Fixes
Tests