Repository navigation
feat(runtime-host): add durable event-wait observation - #6113
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed range: first review of 47c1ede (one commit). Its merge base with main is 1751c39, which is the current main tip, so every line of the diff is the author's.
Shape and scope. About 2.1k of the +7861 lines are product code: event-wait-manager.ts (1329), event-wait-adapter.ts (119), and the core/storage changes (event-wait decode/transition, authority port, SQLite and memory stores, schema migration). Most of the remaining ~5.7k lines are tests, including a 4784-line event-wait-manager.test.ts. There are no generated files. The PR refs tracking issue #5926 and Discussion #5920, and it is the planned "PR2" slice after #6057. Both the issue and the discussion were written by the author, and I found no maintainer feedback on either. Nothing in production code constructs HostEventWaitManager yet, so it is unused at runtime until PR3. P3 (scope): the storage contract (observation checkpoint, source cooldown/dispatch table, schema v16) could land separately from the 1.3k-line manager. That would make each half reviewable. This is a process point, not a runtime defect.
Protocol/epoch. No runtime-host protocol files are touched. RUNTIME_HOST_COMPATIBILITY_EPOCH is 222 on both main and head, so the epoch is unaffected. The persisted workflow schema moves from 13 to 16. Older builds refuse the newer database through the existing "newer than supported" check, which is the right failure mode for records that now carry observation. P3: versions 14 and 15 are skipped and never existed on main. Please use 14 unless there is a reason to skip.
Findings
-
P2 – PR-caused CI failure (flaky new tests). The
testjob fails in@maka/runtime-hosttest:diston the new testwithout storage failures durable observation age remains ahead of lexical ID order(actual: 'waiting', expected: 'resolved'). I reproduced it locally: it passes on an idle machine and fails 4 of 6 runs on a CPU-loaded one. When I replaced the assertion afterclock.advance(30)witheventually(...), waitaresolved every time while the fake clock stayed at 1030, so the product logic is correct and the test is timing-dependent. The cause is the fixture:settle()is a fixed 40setImmediateturns, but each observation goes through filesystem-backed lease checks. Under load, the full file also failed four other tests in one run:source boundary read with response first (0 microtasks) preserves one cooldownMemory: mixed-source pagination observes every peer before expirypre-read authorization Retry-After survives failed dispatch completion (cooldown write fails=true)claim failures preserve turns for a healthy different source peer (7 failed waits)
Please make assertions that follow
clock.advancewait for the expected durable state (aseventuallydoes), or drain to quiescence, rather than relying on a fixed number of event-loop turns. -
P2 (latent) – the default policy exhausts the attempt budget well before the default maximum lifetime. Every observation, including a healthy
pendingpoll, claims one attempt (attempts + 1in#observe). A pending result reschedules afterpollIntervalMs× (1 + jitter), about 15–18 s with the defaults. WithmaxAttempts: 10_000, a wait that stays pending is force-resolved after roughly 42–50 hours assource_unavailable(observation_budget_exhausted).maxLifetimeMsallows 7 days. Notification-driven reads use the 1 snextObserveAtpath and exhaust the budget even faster. Any wait registered for more than about two days therefore ends in a spurious failure even though its source is healthy. The constructor's consistency check does not catch this. Either size the budget to the lifetime (for example, requiremaxAttempts * pollIntervalMs >= maxLifetimeMs), or count only failed or unknown attempts against the budget. The impact is latent because nothing wires the manager yet, but these defaults are the contract PR3 will consume. -
P3 – an adapter-supplied Retry-After has no upper bound and persists across restarts.
#validateResultonly requiresretryAfterMsto be a non-negative safe integer.#recordSourceCooldownthen persistsnow + retryAfterMsinto the source-wideworkflow_event_wait_source_cooldownsrow, whichextendSourceCooldownonly ever extends withMAX. One bad value, such as epoch seconds parsed as a delay, would block every wait on that provider and connection until they all expire. Restarting does not clear it, and there is no reset path. Clamping tomaxBackoffMsor the wait deadline would bound it.
What I checked and found sound. The claim-before-read checkpoint (checking plus a reserved nextPollAt) and the strict transition rules (attempts advance by one only under a checking reservation, the cursor never advances on unknown, resolution after the deadline is rejected, priorResolution and the observation stay frozen when a resolved wait is cancelled) give sensible crash recovery: a crash between the source read and the commit leads to a fresh, budgeted re-read. The durable per-source dispatch reservation (token plus bounded dispatch_until_ms) blocks a restarted host for at most readTimeoutMs + minIntervalMs. The memory and SQLite cooldown/reserve/complete semantics match. Cooldown-table cleanup is bounded per write. In-memory maps (flights, lingering calls, late results, cooldown cache, pending cooldowns, subscriptions) are each bounded by policy. Cancellation, expiry and shutdown abort flights. Late results are still gated by revision CAS, and shutdown is bounded by closeTimeoutMs.
CI and mergeability. GitHub reports the PR as MERGEABLE, but merging is BLOCKED by the missing review. The only failing check is test, which is finding 1. Build, packaging, installed-CLI validation, audit and label all pass.
| [z.waitId], | ||
| ); | ||
| await f.clock.advance(30); | ||
| assert.equal((await f.read(a))?.status, 'resolved'); |
There was a problem hiding this comment.
P2: this assertion is timing-dependent and is the current CI failure. Under CPU load, advance(30) plus a fixed 40-turn settle() can return before the fs-backed lease checks finish. Wait a does resolve without further clock movement if you poll with eventually(...). The same pattern also made four other tests in this file fail under load.
| maxBackoffMs: 300_000, | ||
| readTimeoutMs: 30_000, | ||
| closeTimeoutMs: 5_000, | ||
| maxAttempts: 10_000, |
There was a problem hiding this comment.
P2 (latent): every observation, including a healthy pending poll, consumes an attempt. At about 15–18 s per poll, 10,000 attempts run out after roughly 42–50 hours, but maxLifetimeMs allows 7 days. A long but healthy wait is then force-resolved as source_unavailable / observation_budget_exhausted. Consider requiring maxAttempts * pollIntervalMs >= maxLifetimeMs, or counting only failed or unknown attempts.
| ): Promise<void> { | ||
| if (this.#closed) return; | ||
| const now = Math.max(r.updatedAt, this.#clock.now()); | ||
| const retry = result.kind === 'unknown' ? (result.retryAfterMs ?? 0) : 0; |
There was a problem hiding this comment.
P3: retryAfterMs is unbounded here, and now + retry is persisted source-wide with MAX semantics. A single bad adapter value can block the provider/connection for every wait, survive restarts, and has no reset path. Consider clamping it to maxBackoffMs or the wait deadline.
|
|
||
| export const SQLITE_WORKFLOW_SCHEMA_VERSION = 13; | ||
| // v16 indexes durable observation order so bounded schedulers need no per-wait fairness cache. | ||
| export const SQLITE_WORKFLOW_SCHEMA_VERSION = 16; |
There was a problem hiding this comment.
P3: this jumps from 13 to 16, but versions 14 and 15 never existed on main. Unless there is a reason to skip them, 14 would keep the version history contiguous.
Wait for durable observation and cooldown completion in the flaky test paths, and finish pagination observations before advancing the clock past their deadlines. Keep ordering, accounting and source-limit assertions. Consolidate the unreleased workflow additions into schema 14. Cover the 13-to-14 upgrade, retained wait bytes and source reservations on reopen, and rejection of newer development databases without changing wait data. Document independent attempt/lifetime limits and normalized source-wide Retry-After semantics. Add healthy-pending budget coverage for both stores and assert that source cooldowns are not shortened by local backoff caps. Validated 370 related tests, controlled failing-before/passing-after synchronization, six CPU-loaded rounds of 12 focused tests, final schema checks, root lint/format/build/typecheck and Desktop/UI Knip. Refs: apache#6113 Generated-by: OpenAI Codex
|
Thanks for the review. The follow-up commit
Agreed. The failing CI assertion assumed that The affected tests now wait for the durable observation or source cooldown they assert. The mixed-source pagination test also waits for completed observations between clock advances, so real filesystem scheduling cannot move an unfinished observation past its simulated deadline. The ordering, attempt accounting, single cooldown write, and no-early-read assertions remain in place; no sleep, retry-count, or timeout was increased. A controlled finite storage delay makes the original assertion fail with
Your estimate is correct: with a promptly responding source and ordinary polling, 10,000 attempts at roughly 15–18 seconds each can run out after about 42–50 hours. The seven-day value is an independent lifetime ceiling, however, rather than a promise that every accepted wait can keep observing for seven days. The published PR2 plan explicitly maps an exhausted observation budget to The stored reason is I have kept the independent limits. Counting only failed/unknown observations would turn the total observation budget into an error budget and remove its cap on healthy source requests. Requiring
Agreed on the failure mode: an incorrectly normalized, excessively large adapter value can persist a long provider/connection cooldown. The adapter contract now explicitly requires a relative millisecond delay and normalization of source timestamps and units before returning it. I have not clamped the shared limit to The Local/Memory tests now explicitly assert that the persisted source delay exceeds the local backoff cap, in addition to their existing cancellation/expiry/purge, restart, connection-isolation, and no-early-read checks. A separate diagnostic or recovery policy for invalid adapter output remains a possible follow-up; the comment clarification does not claim to detect every incorrect but numerically valid delay. There are no production adapters in this PR, and any eventual source-specific normalization needs its own tests.
Changed to 14. The observation indexes, source cooldowns, and dispatch reservation columns are now created together in the single upgrade from main's version 13. The intermediate 14/15 migration tests have been replaced with coverage of 13-to-14 migration and reopening the final v14 schema, retaining wait bytes, cooldowns, and dispatch ownership. The newer-version guard is preserved. Databases marked 15 or 16 by earlier development snapshots are rejected without rewriting their version or wait records; this change does not silently downgrade them.
I agree that the size deserves attention. I have kept this as the planned PR2 slice because the minimal observation persistence and Host manager together demonstrate the fake-source observation/recovery flow specified in the issue. Splitting them is a review-process option, rather than a correctness fix, and I have not introduced that restructuring in this follow-up. The implementation plan is not being presented as maintainer approval of the design. Validation: the related contract, authority, migration, bundle-isolation, operational-store, and manager suites passed 370/370. After strengthening the future-version rejection check, the four affected schema tests passed again on the final build. The controlled before/after synchronization comparison and all six CPU-loaded focused rounds passed as described above. Root lint, format, build, and typecheck, plus Desktop/UI Knip, passed. Full-repository tests and Desktop E2E were not run; CI validation is still needed on the pushed follow-up. |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed 846850f05048714d4e9459876d0e2b51d8efad11 as a follow-up to 47c1edef: one additional commit, five files, +182/-70, with no intervening merge or rebase. The flaky-test P2 is addressed. The budget behavior has not changed; I would retain that concern as P3 under the newly explicit independent-limit contract, not describe it as a runtime fix. I found no new P0–P2 issue in this increment.
Test synchronization. The affected assertions now wait for durable observations or cooldowns rather than assuming a fixed number of event-loop turns completes filesystem-backed work. Pagination finishes each observation before moving the simulated clock past its deadline. In a controlled finite-write-delay test, the revised lexical-order test passes without another clock advance; removing only its added eventually restores the original waiting versus resolved failure. Three CPU-loaded rounds of the affected test families passed, 13 tests per round. This supports the fix without claiming the tests can never be flaky.
P3 — document the effective default observation horizon. At packages/runtime-host/src/server/event-wait-manager.ts:83–88, the defaults remain 15-second polling, 10,000 total attempts and a seven-day maximum lifetime. For a promptly responding healthy source that stays pending, ordinary 15–18-second polls consume that budget in approximately 42–50 hours, before reaching the seven-day ceiling. Notification-triggered reads may consume it faster. The new comments and tests explicitly preserve this behavior: exhaustion resolves the wait as source_unavailable with reason observation_budget_exhausted. I verified the default-budget boundary through both Local and Memory storage paths by retaining 9,999 prior observations and performing the final healthy read. Please either align the defaults for the intended polling horizon or explicitly document the approximate two-day effective horizon. Merely saying the limits are independent does not communicate that default behavior. The appropriate production-source defaults remain a policy choice, not validated by this test.
Migration. The unreleased workflow additions are consolidated from main's version 13 into version 14; intermediate versions 15/16 are not silently downgraded. I created a genuine v13 database using the actual main storage implementation, including a Session and durable wait, then opened it with this head. Existing table contents and wait bytes were preserved; the observation indexes and source dispatch columns were present. Reopening v14 retains cooldown/reservation facts in the regression tests. Databases marked 15 or 16 are rejected without changing their version or contents. I found no migration-order or data-loss regression.
Current CI failure is pre-existing. Attempt 1 of run 38823986212 failed in the existing allows concurrent operational owners to initialize the same fresh WAL database test, with New SQLite runtime database did not enable incremental auto-vacuum, not the earlier observation assertion. Initialization still sets auto-vacuum before entering migration (operational-state-store.ts:229 versus workflow migration at line 496). With two real SQLite-opening processes, I reproduced the same failure on both this head and the actual main storage code: one opener sees an empty file, while another sees that file as already existing and creates its schema before the first opener sets auto-vacuum. This failure is not introduced by the workflow consolidation. The failed job has been rerun; attempt 2 is currently in progress, so this is not a green CI result.
The clean workspace build, core/storage/runtime-host type checks, checks for the five changed files, 370 related contract/store/manager/bundle tests, and protocol epoch guard passed. There are no protocol changes against main 1751c399; epoch 222 is unchanged. GitHub reports the branch as mergeable but blocked. I did not rerun the full repository suites, Desktop E2E, or production source integrations.
Document the roughly 42-50 hour polling budget for a promptly responding, continuously pending source and the source_unavailable exhaustion result. Explain how notifications, scheduling, I/O, contention and throttling affect elapsed time, separately from the seven-day lifetime ceiling. Keep default values and runtime behavior unchanged. Validated root lint, format, build and typecheck, Desktop/UI Knip, and both existing Local/Memory healthy-pending budget tests. Refs: apache#6113 Generated-by: OpenAI Codex
|
Thanks for the follow-up review. Agreed that describing the limits as independent did not make the effective default observation horizon clear. Addressed the remaining P3 in 7b49336c8. The documentation beside
This is a documentation-only change; the defaults and runtime behavior are unchanged. Validation: root lint, format check, build and typecheck, plus Desktop/UI Knip, passed. Both existing Local/Memory healthy-pending budget tests passed against rebuilt output. Full-repository tests and Desktop E2E were not rerun for this change. |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 846850f0..7b49336c, one documentation commit that adds a comment above DEFAULT_EVENT_WAIT_OBSERVATION_POLICY (packages/runtime-host/src/server/event-wait-manager.ts). No code changes, and I found no P0–P2 issue.
- This resolves our earlier P3. The comment says the default attempt budget, not the seven-day lifetime, ends a healthy pending wait after about 42–50 hours, and that exhaustion resolves as
source_unavailable/observation_budget_exhausted. The numbers match the defaults: 10,000 attempts at 15–18 s polling is about 41.7–50 h. It also notes that notification-triggered reads can use up the budget sooner, which matches the attempt accounting.
The CI test failure is not caused by this PR. check:artifact-budget fails because node_modules/marked@18.0.11 is missing from the desktop production-dependency ledger. Main fails the same way, starting at d80d6a8 (#5455): #5969 added marked before that ledger was recorded. #6129 refreshes the ledger. A re-run should pass once main is fixed, or after a merge or rebase onto the fixed main.
Add bounded observation checkpoints, status queries and an internal Host wait manager with source adapters, polling, notification reconciliation, backoff, cancellation and restart recovery. Preserve the existing wait CAS authority and keep model execution and delivery in the later bridge. Migrate workflow storage from 13 to 16 while retaining existing records. Persist shared source limits across interrupted observation and process normal and late results through an exclusive handoff. Recheck source cooldowns and refresh authorization before deferred subscription setup. Cover source races, expiry, lost acknowledgements, cancellation handoffs and provider parity with controlled-clock tests. Refs: apache#5926 Generated-by: OpenAI Codex
Wait for durable observation and cooldown completion in the flaky test paths, and finish pagination observations before advancing the clock past their deadlines. Keep ordering, accounting and source-limit assertions. Consolidate the unreleased workflow additions into schema 14. Cover the 13-to-14 upgrade, retained wait bytes and source reservations on reopen, and rejection of newer development databases without changing wait data. Document independent attempt/lifetime limits and normalized source-wide Retry-After semantics. Add healthy-pending budget coverage for both stores and assert that source cooldowns are not shortened by local backoff caps. Validated 370 related tests, controlled failing-before/passing-after synchronization, six CPU-loaded rounds of 12 focused tests, final schema checks, root lint/format/build/typecheck and Desktop/UI Knip. Refs: apache#6113 Generated-by: OpenAI Codex
Document the roughly 42-50 hour polling budget for a promptly responding, continuously pending source and the source_unavailable exhaustion result. Explain how notifications, scheduling, I/O, contention and throttling affect elapsed time, separately from the seven-day lifetime ceiling. Keep default values and runtime behavior unchanged. Validated root lint, format, build and typecheck, Desktop/UI Knip, and both existing Local/Memory healthy-pending budget tests. Refs: apache#6113 Generated-by: OpenAI Codex
7b49336 to
bd20d02
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.Incremental review:
7b49336c..bd20d025. This is a pure rebase onto main2c429077(#6129).git range-diffshows all three author commits as patch-identical (47c1edef→697e04df,846850f0→9721b9a0,7b49336c→bd20d025), with no hand-resolved conflicts. My conclusions from7b49336cstill hold: no P0–P2 issue, and the earlier P3 about the effective observation horizon is resolved by the documentation commit.
The earlier CI failure (check:artifact-budget, marked@18.0.11) came from main and is fixed by #6129, which this head now includes. CI test was still running when I posted this.
The event-wait tests can exhaust their fixed `setImmediate` polling budget before asynchronous Host work reaches the expected checkpoint. In CI this caused the late-authorization and slow-cooldown-persistence tests to fail before their third authorization observation. Give the test helper a 5-second monotonic wall-clock deadline with 5 ms sleeps. The injected observation clock remains fixed, so existing timeout, Retry-After and cancellation assertions retain their exact semantics. A regression test verifies that delayed Host work completes without advancing that clock or firing its timers. Production code is unchanged. Extracted from #6089 following its scope review, so this test fix can land independently of Write/Edit evidence. Refs #6113. Failure evidence: https://github.com/apache/maka/actions/runs/39031806684/job/117393589335 Generated-by: OpenAI Codex
Summary
PR1 introduced durable wait records. This PR adds an internal Host manager that observes those waits and persists condition outcomes without invoking the work model.
With fake adapters, this covers discovering a wait, observing an already-satisfied or changing condition, recording the result, and recovering after restart. Production source integrations follow in later PRs; tool yield, Goal continuation, and successor admission belong to PR3.
Refs #5926 — PR2: Host observation and condition management.
Design context: Discussion #5920 — Condition-driven waiting and wake-up.
Verification
git diff --check.Full repository checks and Desktop E2E were not run. Production adapters and PR3 execution admission are outside this PR's validation scope.
AI use
Tool(s) and scope: OpenAI Codex assisted with implementation, tests, regression fixes, and review. The commit includes a
Generated-by: OpenAI Codextrailer.Checklist
Does this PR entail a change in behavior?