feat(node-manager): Task Layer Increment 3 — group-key rotation - #4213
Conversation
…tart/drift) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Thread revertOf into the persisted seed passed to TaskRegistry.create so the revert task's first persisted record already has it, instead of patching it onto the live task object after run() has already tracked/driven it.
The comment implied these assertions prove the pre-fix post-hoc assignment was racy; they don't discriminate old vs new code in the current call graph (revertOf is set synchronously before the revert's own first persist either way). Reword to describe the invariant actually being checked.
RunningTaskContext only ever calls itemKind() and reconcile() on the reconciler it's given, not the rest of ReconcilerBehavior's surface. Type the ctor param against a new 2-method ReconcilerSurface interface instead, so fakes in tests can implement it directly instead of casting through `as unknown as ReconcilerBehavior`. ReconcilerBehavior satisfies the new interface unchanged.
…ine late cancel) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Tick the box to add this pull request to the merge queue (same as
|
There was a problem hiding this comment.
Pull request overview
Adds controller-driven, multi-node group-key rotation with phased convergence, restart recovery, and drift correction.
Changes:
- Adds distribute, activate, and cleanup rotation phases.
- Updates group-key reconciliation to compare epoch start times.
- Adds task rollback controls and comprehensive integration tests.
Verification: PR reports build, formatting, lint, and all relevant tests passing.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/node-manager/src/reconcile/GroupKeyItemKind.ts |
Adds write-if-start-set-differs behavior. |
packages/node-manager/src/reconcile/ReconcilerSurface.ts |
Defines the task-facing reconciler interface. |
packages/node-manager/src/task/errors.ts |
Adds the non-revertible-task error. |
packages/node-manager/src/task/groups/RotateGroupKey.ts |
Implements phased group-key rotation. |
packages/node-manager/src/task/RunningTaskContext.ts |
Adds intent-based peer discovery. |
packages/node-manager/src/task/Task.ts |
Adds task revertibility metadata. |
packages/node-manager/src/task/TaskManagerBehavior.ts |
Registers rotation and controls rollback. |
packages/node-manager/src/task/types.ts |
Extends the task context contract. |
packages/node-manager/test/GroupKeyIntegrationTest.ts |
Updates key-set drift integration coverage. |
packages/node-manager/test/reconcile/GroupKeyItemKindTest.ts |
Tests start-time diff reconciliation. |
packages/node-manager/test/task/AdmissionTest.ts |
Updates typed item-kind test doubles. |
packages/node-manager/test/task/ChangesetTest.ts |
Uses the narrowed reconciler interface. |
packages/node-manager/test/task/groups/RemoveNodeFromGroupTest.ts |
Removes obsolete reconciler casts. |
packages/node-manager/test/task/groups/RollbackIntegrationTest.ts |
Tests seeded rollback metadata and idempotency. |
packages/node-manager/test/task/groups/RotateGroupKeyIntegrationTest.ts |
Tests two-member rotation and recovery scenarios. |
packages/node-manager/test/task/groups/RotateGroupKeyTest.ts |
Tests single-member phase behavior. |
packages/node-manager/test/task/helpers.ts |
Strengthens fake reconciler typing. |
packages/node-manager/test/task/PeersWithIntentTest.ts |
Tests live-intent peer discovery. |
packages/node-manager/test/task/RemoveIntentTest.ts |
Removes obsolete reconciler casts. |
packages/node-manager/test/task/RevertTaskTest.ts |
Removes obsolete reconciler casts. |
packages/node-manager/test/task/TaskContextGateTest.ts |
Removes obsolete reconciler casts. |
packages/node-manager/test/task/TaskManagerBehaviorTest.ts |
Tests revertibility and rollback suppression. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…R review) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… rotation) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
packages/node-manager/src/task/TaskManagerBehavior.ts:212
- Cancellation can still cross the activation point after this check. If cancel runs during distribute's initial gate reconciliation, it passes
revertible, sets the gate abort, and waits; when that in-flight reconciliation succeeds,awaitGatereturns without rechecking the abort, so#drivecan enter activate and write activate intents before the next gate observes cancellation. Mark a running/parked task cancelled before waking the gate so the drive loop cannot start another phase.
// A task past its point of no return declines cancel with zero side effects (gate untouched, state kept).
if (!task.revertible) {
throw new TaskNotRevertibleError(`Task ${task.id} is not revertible: ${task.notRevertibleReason}`);
}
packages/node-manager/src/task/groups/RotateGroupKey.ts:77
groupKeySetId === 0is explicitly unsupported byGroupKeyItemKindand deferred by this PR, but the empty-member fast path makesrotateGroupKeyfor keyset 0 complete successfully without doing anything. Reject keyset 0 before member lookup so callers do not receive false success for an unsupported IPK rotation.
const key = String(this.params.groupKeySetId);
const members = ctx.peersWithIntent("groupKey", key);
if (members.length === 0) {
return;
…e, refuse loudly) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Node Manager — Task Layer Increment 3: group-key rotation
Controller-driven, gap-free, clock-independent rotation of a group operational key across all member nodes, via a
RotateGroupKeytask built on agroupKeykind that can now update (not just create) a keyset.Sub-PR into
node-manager(the umbrella branch). Scope is group-key rotation only; IPK (keyset 0) rotation is deferred to a later increment (it feeds CASE — mis-rotation is an unrecoverable brick, needs its own design discussion).What's in it
groupKey.apply→ write-if-set-differs (supersedes Inc2a create-if-absent). Key material is unreadable, butKeySetReadreturnsepochStartTimes, so we diff the device's start-time set against the intent's and write the full struct when they differ (absent = empty set). A same-set re-apply is a no-op. This is what lets a rotation phase (a different, intended struct) actually land.TaskContext.peersWithIntent(kind, key)— derives the member set (every peer holding a live desired-state item for the keyset).RotateGroupKey— three gated phases: distribute the new key far-future-dormant → activate it (old key still present to decrypt in-flight) → drop the old key. Each phase blocks until all members commit; an offline member parks the task. Only the reconciler touches devices.revertOfset at task creation (not patched post-run);ReconcilerSurfaceinterface narrowsRunningTaskContext's reconciler param and drops the test-side casts.Rotation is forward-only once realized
A realized group-key rotation cannot be undone — the intent is "replace the key"; if a rotation turns out bad, the remedy is to rotate to another key, never restore the retired one. Mechanically it must be forward-only anyway: cleanup back-dates the surviving key to the original start time, so a completed rotation's start-time set equals the pre-rotation set, and a revert (which only diffs start-time sets) would be a silent no-op that falsely reports success. So
RotateGroupKeyis revertible only before it enters cleanup; past that pointcancel()declines withTaskNotRevertibleErrorand auto-rollback is suppressed (a mid-cleanup failure self-heals — the cleanup intent is already set on every member, so the reconciler converges them forward).Notes on the implementation vs. the original design
Two design-doc assumptions were wrong and were corrected against the real device code (caught by real-device integration tests):
epochStartTimeis unix-µs in matter.js (FabricGroups.addGroupEpoch=Time.nowMs * 1000; the device floor isIPK_DEFAULT_EPOCH_START_TIME= 2000-01-01 unix-µs), not matter-epoch µs. No offset subtraction.KeySets.ts: newest key withstartTime <= deviceNow), not the spec's second-newest rule (that is IPK-only). Cleanup therefore back-dates the survivor to a firmly-past time so it is selectable on any device clock.Follow-ups (not in this PR)
setsEqual/startsOfcompare as multisets (no dedupe); no live bug since rotation never emits duplicate start-times.Gates
build --clean·format-verify·lint· node-manager 141/141 · node 1469/1469 — all green.🤖 Generated with Claude Code