feat(app): process roles - #622
Conversation
mq.backend, cache.backend, dedupe.backend and coord.backend select each layer's implementation; only today's in-process one exists per layer and it is the default. Validate refuses an unknown value, internal/app picks the implementation in one switch per layer, data_dir is probed only when a selected backend keeps state there, and boot logs Config.Warnings. Part of #613. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
New internal/coord: Coordinator/TryAcquire/Term with a fencing Token, Done/Err and Resign; RunElected for leader loops; Local, the in-process implementation; and coordtest.Conformance, the suite every backend runs. The sweeper now runs through RunElected under the "sweeper" lease, over a Local coordinator that wireCoord opens until coord.backend lands. Part of #613. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
A handoff overlap cannot lose ClickHouse data (every sweep stops at the ack floor) but can trim SSE replay history when the holders' settings views differ. Also lists coord/ in development.md's package tree. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
…ENTS.md Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
wireCoord becomes a switch on coord.backend like the other layers, and New refuses a Config that names no coordinator. Docs stop calling the key reserved. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
roles (WH_ROLES, default api,ingest,sweeper) picks which components a process wires, and instance_id (WH_INSTANCE_ID, default <hostname>-<8 hex>) names it. Discovery, dedupe, the token verifiers, the hub bridge and keepalive stay per API process; the ingest worker is the ingest role; the sweeper is the sweeper role and stays lease-elected through a.elected. A process without api serves an ops-only router: probes, /version, metrics, and the settings reload behind the operator key alone. Boot refuses any split over the embedded MQ, and api without ingest (or the reverse) over a local cache. Part of #613. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
Review round 1: instance_id is only logged until a shared coord.backend records it; sweeper exclusivity across processes needs a shared coord.backend; the ops listener serves the probe aliases and answers 403 before 404 under /v1/ops; architecture.md's config and router sections cover roles and NewOpsRouter. The YAML roles test uses a non-default order so it can tell the file from the env default. Part of #613. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (20)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (8)See [AGENTS.md](AGENTS.md) for project conventions, architecture notes, and AI agent instructions.📄 CodeRabbit inference engine (CLAUDE.md) Files:
Source excerpt: Register the route in `internal/api/router.go`.📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: Create or modify a handler in `internal/api/` (follow existing patterns like `ingest.go`).📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: Create `*_test.go` files in the same package as the code under test.📄 CodeRabbit inference engine (AGENTS.md) Files:
See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Source excerpt: Create the package under `internal/`.📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: **In MDX, leave a blank line between a JSX tag and a code fence.**📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: Document in `docs/src/content/docs/architecture.md`.📄 CodeRabbit inference engine (AGENTS.md) Files:
🧠 Learnings (1)📚 Learning: 2026-06-26T12:23:22.696ZApplied to files:
🪛 LanguageToolCHANGELOG.md[typographical] ~13-~13: Consider using an em dash in dialogues and enumerations. (DASH_RULE) docs/src/content/docs/configuration.mdx[style] ~59-~59: To elevate your writing, try using an alternative expression here. (MATTERS_RELEVANT) [style] ~64-~64: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~64-~64: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~66-~66: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) docs/src/content/docs/architecture.md[typographical] ~80-~80: Consider using an em dash in dialogues and enumerations. (DASH_RULE) [style] ~94-~94: This phrase is redundant. Consider writing “last”. (LAST_OF_ALL) [style] ~94-~94: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) docs/src/content/docs/deployment.md[style] ~340-~340: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~340-~340: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) AGENTS.md[typographical] ~27-~27: Consider using an em dash in dialogues and enumerations. (DASH_RULE) [grammar] ~27-~27: Please add a punctuation mark at the end of paragraph. (PUNCTUATION_PARAGRAPH_END) 🔇 Additional comments (20)
📝 SummarySummary by CodeRabbit
WalkthroughThe change adds boot-configured API, ingest, and sweeper roles, role-aware component wiring, and instance IDs. Processes without the API role receive an ops router for probes, version, metrics, and operator-key settings reload. Configuration rejects role splits incompatible with embedded MQ or local cache. ChangesProcess roles and topology
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ConfigLoad
participant appNew
participant wireOpsHTTP
participant NewOpsRouter
participant MainServer
ConfigLoad->>appNew: provide validated roles and instance ID
appNew->>wireOpsHTTP: configure HTTP when the API role is absent
wireOpsHTTP->>NewOpsRouter: provide health, version, settings, auth, and metrics
wireOpsHTTP->>MainServer: register the ops handler
MainServer->>NewOpsRouter: route probes, version, metrics, or settings reload
NewOpsRouter-->>MainServer: return the route response
Merge Risk: ⚪ Minimal · up to No identified issue blocks merging; role validation and the configured metrics listener remain intact. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 70.59% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 13 files. (7 skipped: 7 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
Sync #615 with its parent, which now includes main's #612 squash. Resolved a conflict in docs/architecture.md (app/wiring section): kept boot-backends' rewritten prose and combined it with coord-leases' own additions (the lease coordinator in app.go's boot order, `wireCoord` in wire.go's backend switch, and the sweeper-lease clause). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sync #622 with its updated parent (#615, now synced with main through #612). Resolved conflicts in AGENTS.md and docs/architecture.md (app/wiring sections): kept coord-leases' rewritten prose (which already folds in main's own edits) and combined it with process-roles' own additions (the roles-aware app.go boot order and the `elected` wrapper around RunElected in wire.go's sweeper-lease clause). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Brings in main via feat/coord-leases: #618's squash, #619, #632, #616, #655 and #647. Per #632, the roles default moves from its env-default tag into defaults(): an explicit `roles: []` now reaches Validate (a refusedZeros entry pins it), and the doc-defaults test parses the roles cell as a comma-separated list. instance_id's documented default is *(empty)*, the value in defaults(); Load resolves it to <hostname>-<8 hex>. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
📚 Docs preview is live → https://7cfe8491-wavehouse-docs.wave-rf.workers.dev
|
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit ada4bdd in the Show a line coverage summary of the most impacted files.
Updated |
Over a nested settings directory there is no watcher, so a process without the api role and without an operator key reloads by SIGHUP alone; the warning said "or the directory watcher". TestNew_OpsOnlyRouter opened the sweeper-only app on the same data dir as the still-open full app, pointing two embedded JetStream servers at one store. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GW5hTHhJoY3t4dbeoqkEGQ
AGENTS.md: main's api/ entry (ch_errors.go) beside this branch's app/ entry; each side had changed only its own line. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GW5hTHhJoY3t4dbeoqkEGQ
#627 added a hand-built Config after this branch made an empty roles refuse boot, as setup_test.go and tenants_test.go already do. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GW5hTHhJoY3t4dbeoqkEGQ
main now carries #612 and #618 as squashes, plus #632, #622, #627, #615, #619, #647, #616, #655 and #623. The merge was resolved against the pre-squash #618 head (f129d57) as its base, so main's version wins for everything this stack does not own and only the cache stack's changes (#614, #621, #626 as merged here, and this PR) are re-applied on top. Warnings keeps main's api-role gate for the cache.redis warnings too: a split's Deployments differ only in roles, so the API's cover the others'. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
… cache app.New now wires the API, ingest and cache only for the roles a config names (#622), and refuses a Config with none, so the shared-cache integration test's instances run every role, as the suite's own app does. The refusal of api without ingest over a local cache now names cache.backend=redis, the shared cache it asks for, and the configuration and deployment pages say this build has a shared cache but still refuses every split while the queue and leases are in-process. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
Absorb feat/dedupe-windowed-ingest's newer history, including its own merge of feat/dedupe-reserve (step 1 of this pass) and origin/main (#615 leases, #618 backend selection, #622 process roles, #627 ClickHouse error classing, and the rest through #623). Resolved six conflicts, combining both sides' facts rather than picking one: - internal/settings/settings.go: kept MinDedupeRetention (this PR's 2-minute floor, tied to the embedded queue's duplicate window) next to windowed-ingest's reworded DLQConfig comment ("a row ClickHouse still rejects", reflecting the outage-retry split from #613). - AGENTS.md: kept this PR's dedupe/ package-inventory line (the sweep detail) alongside windowed-ingest's updated config/ and new coord/ lines pulled in from main. - CHANGELOG.md, docs/architecture.md, docs/durability.md, docs/settings-directory.mdx: superseded this branch's now-stale copies (old `evt%2D123` key spelling from before refactor/keyenc kept '-'; the metric name `wavehouse_dedupe_commit_failed_total` before it gained the `ingest_` prefix; the simpler "fits inside" duplicate-window wording before the `2×lease+1s` invariant was pinned) with windowed-ingest's current, code-matching text, then folded this PR's retention-specific additions back in: the Upgrade note now says the pre-#222 keys are "deleted by the retention sweep" instead of "nothing removing them yet", and durability.md's duplicate-window paragraph keeps its closing sentence tying `dedupe.retention`'s 2-minute floor to that same window. internal/api/ingest.go, ingest_test.go, ingest_window_test.go, app/wire.go, app/app_test.go, dedupe/embedded_test.go and settings/settings.go (the rest of it) auto-merged with no textual conflict; verified by reading the result rather than trusting that: every Commit path windowed-ingest added (commitClaims before the failing record in publishFailed, and after a clean window in ingestWindow) already passes commitClaims the full pendingRecord slice, and commitClaims groups by each record's resolved retention and issues one Commit per distinct value — so both PRs' Commit-path changes compose correctly. The sweep's commitMu (embedded.go/sweep.go) and Managed.Apply's fast path (managed.go) touch disjoint locks and did not need reconciling. go build, go vet -tags integration (whole repo), and go test -race across internal/dedupe, internal/settings, internal/api and internal/mq all pass (re-run with -count=1 after one flaky timing assertion in TestEmbedded_SweepChunkOverTombstonesDoesNotHoldCommits — a 100ms budget the sweep raced past once under parallel-package load — passed clean on every subsequent run, including three solo runs and a full fresh run; unrelated to this merge). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Part of #613. This is PR C1 of the #613 core design (process roles). Stacked on #615 (B1), which is stacked on #618 (G1).
What
roles/WH_ROLES(defaultapi,ingest,sweeper) andinstance_id/WH_INSTANCE_ID(default<hostname>-<8 hex>, with a fresh suffix at every boot) ininternal/config/config.go, plusConfig.Has(Role)andconfig.AllRoles(). Entries are trimmed at Load. An empty list, an empty entry, an unknown role, or a role named twice refuses boot (rule 1 for roles).Validate(validateTopology):mq.backend=embeddedis refused. Until D ships, every split is refused, which is expected.apiandingestwithcache.backend=localis refused. See the deviation note below.app.New:server.port.apiadds schema discovery, the dedupe stores, streaming (hub, hub bridge, keepalive), auth and the full router. These stay per API process.ingestadds the ingest worker.sweeperadds the sweeper. It stays lease-elected through the newa.elected(lease, fn)wrapper overcoord.RunElected.apioringest.Newrefuses aConfigwith no roles. Only one built withoutconfig.Loadcan have none, so it follows G1's rule that the zero value is not the default. The three hand-built configs now setconfig.AllRoles().api.NewOpsRouter, sharingnewProbeRouterwithNewRouter) for a process withoutapi:/livez,/readyz(and their aliases),/version, the same-port metrics path, andPOST /v1/ops/settings/reload. Every tenant route and the rest of/v1/opsanswer 404.Authenticator(wireOpsAuth) andRequireAdmin(nil), the same gate as/v1/ops/*over a nested directory. No token verifier or JWKS fetch runs withoutapi, so an admin JWT gets 401 there./readyzpings the pools in an ingest process. A sweeper-only process is ready once it has booted.NeedsDataDirprobes for Pebble only when the process runsapi. The two shared-queueWarningsare skipped withoutapi, because onlyapiopens a cache it reads or a dedupe store.config.yaml.Deviations and additions to the design
api,ingestreplica with a local cache plus asweeperprocess.api+sweeperandingest+sweeperare still refused.rolesentries are trimmed at Load, soWH_ROLES="api, ingest"works. The design's[unverified]note on cleanenv's,separator is now pinned byTestLoad_RolesFromEnv.Ingest scaling (reconciliation.md)
C1 wires the ingest worker exactly as today: one consumer per ingest process on the shared durable, so N ingest processes are competing consumers (the MVP). Nothing here rules out shard claiming later. C5/D5 can wrap per-shard consumers in
a.electedorTryAcquire("ingest/shard/<i>")insidewireIngestWorkerwithout touching the role plumbing. The hub bridge stays per API process.Open question: should the settings reload route be served on worker processes?
Will callers need to call
POST /v1/ops/settings/reloadon worker pods too, through the ops-only listener? This PR builds the route there, gated by the operator key as/v1/ops/*is, on the assumption that they will. If reload is only ever needed on API pods, the route can stay: it is harmless, and a worker still reloads on SIGHUP and, over a flat directory, through the watcher.Left to later PRs (by design)
mq.backend=nats(D) and a sharedcache.backend(E).instance_id: B2 uses it. C1 only resolves it and logs it at boot (process rolesline).Tests
internal/config/roles_test.go:instance_idthat is the hostname plus 8 hex, new at every LoadWH_ROLESparsing: trimmed and in any order; one role; empty; YAML listunboundEnvknows both variablesWarningsandNeedsDataDirwithoutapiinternal/app/roles_test.go:a.componentsnamesConfigwith no roles is refused/versionanswer 200; reload is 200 with the operator key, 401 with an admin JWT that the full API admits, and 403 with no key or a wrong key; eight tenant and ops routes answer 404internal/api/router_test.go:TestNewOpsRouter.Verification
make ci(queued,GOTOOLCHAIN=go1.26.6) passed at beab0fd and again at 5de4fd0. That covers unit, integration and e2e tests plus the coverage gates.pre-push-reviewer (opus): round 1
iterate. It found thatinstance_idclaimed to name a lease holder, and that the YAML roles test could not tell the file's list from the env default. Both are fixed in 5de4fd0. Round 2 returned ship_it at 5de4fd0.docs-reviewer (opus): round 1
iterate. It found five problems:config/androuter.gosections were not synced./v1/ops.coord.backend.instance_idwording claimed a lease holder.All are fixed in 5de4fd0. Round 2 returned ship_it.
Known gate gap (fix(agents): pre-push review gate can attest to unreviewed code (marker inherited across commits; wrong worktree checked) #454): the reviewer markers land in the main checkout, so the verdicts are recorded here. No marker was hand-written.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL