Skip to content

refactor(agent): clarify VM reconcile scheduler async contract - #1688

Merged
branarakic merged 2 commits into
mainfrom
fix/vm-reconcile-scheduler-contract
Jul 14, 2026
Merged

refactor(agent): clarify VM reconcile scheduler async contract#1688
branarakic merged 2 commits into
mainfrom
fix/vm-reconcile-scheduler-contract

Conversation

@branarakic

Copy link
Copy Markdown
Contributor

Follow-up to #1682 addressing the final review-agent comment after that PR had already merged.

  • Make live and periodic triggers explicit fire-and-forget enqueue operations.
  • Add waitForIdle(key) as the dedicated completion boundary, including queued trailing passes.
  • Update scheduler coverage for burst coalescing, failure recovery, queued periodic work, and per-CG isolation.

Validation: focused scheduler suite 12/12; agent build and type tests passed.

return this.states.get(key)?.inFlight !== undefined;
}

/** Wait for the active pass and every trailing pass already queued for this key. */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Avoid making test synchronization a public scheduler API

What's wrong
This change removes the misleading promise return from the trigger methods, which is good, but replaces it with a broader public waitForIdle method that appears to exist only so tests can await internal scheduler state. That leaks the scheduler's mutable bookkeeping model into the exported API and gives future callers a vague completion primitive whose behavior depends on concurrent enqueues while it is waiting. Structurally, this is more API surface and more state coupling than the feature needs.

Example
Current shape: production API is triggerLive, triggerPeriodic, isInFlight, and now waitForIdle, even though the only new caller is unit-test synchronization. That makes test orchestration part of the exported scheduler surface.

Suggested direction
Either keep the completion boundary private/test-local, or introduce a real production drain abstraction at the lifecycle layer where shutdown or orchestration would naturally own it. If this public method is intentional production API, add the production caller now so the boundary is justified by the architecture rather than by tests.

Confidence note
The method may be intended for future production shutdown/drain behavior, but in this branch it is only consumed by tests while the production call sites remain fire-and-forget.

For Agents
Look at packages/agent/src/chain-reconciler.ts around VmReconcileScheduler. Preserve fire-and-forget production trigger semantics, but avoid adding a public idle-drain API unless a production lifecycle actually needs it. Consider keeping the trigger path returning an internal/current drain promise for tests, or moving deterministic scheduler-driving helpers into the test harness. Existing scheduler tests should still prove live coalescing, failure hold, periodic recovery, and per-key isolation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining — API-surface preference with no behavior claim (your own confidence note concedes production stays fire-and-forget).

Concretely: every production call site was already fire-and-forget (void this.vmReconcileScheduler.triggerLive(...) at dkg-agent-swm-host.ts:2420/2625/2641, void ...triggerPeriodic(...) at :2540) — nothing awaits them, so Promise<void> -> void changes no production behavior. chain-reconciler.ts is not re-exported from packages/agent/src/index.ts, so this is internal module surface, not published package API.

And the tests do not lean on waitForIdle for their teeth — the runs counters do. waitForIdle follows trailing passes, so a coalescing regression pushes runs past 2 and the assertion fails. It makes those tests strictly stricter, not weaker.

@branarakic
branarakic merged commit 73d86d2 into main Jul 14, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants