feat: classify events and add waitForRun polling fallback (#162 step 2/4)#169
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDurablyEventを「ドメイン」と「オペレーショナル」に分類する型/ランタイム判定（isDomainEvent）を追加し、waitForRun/triggerAndWaitにプロセス外のストレージポーリング用の Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RuntimeA as Runtime A
participant RuntimeB as Runtime B
participant DB as Storage(DB)
Client->>RuntimeA: triggerAndWait(runId, {pollingIntervalMs})
RuntimeA->>RuntimeA: start in-process listener for events
RuntimeA->>RuntimeB: (no direct call) RuntimeB may lease and process run
RuntimeB->>DB: lease run / update status (pending -> leased -> completed/failed/cancelled)
DB-->>RuntimeA: (no push) RuntimeA polls storage at pollingIntervalMs if no terminal event seen
RuntimeB->>RuntimeA: emit in-process event (e.g., run:complete) -> RuntimeA listeners settle immediately
alt another process completed run
RuntimeA->>DB: poll -> sees terminal state -> resolve wait/reject accordingly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
website/api/events.md (1)
14-22: コード例が冗長 —run:failは常にドメインイベントです
durably.on('run:fail', ...)のリスナー内でisDomainEvent(event)を呼び出していますが、run:failはすでにドメインイベントとして定義されているため、この条件分岐は常にtrueになります。より実用的な例としては、全イベントを購読するケースや、イベント配列をフィルタリングするケースが考えられます：
📝 より実用的な例の提案
-durably.on('run:fail', (event) => { - if (isDomainEvent(event)) { - // narrowed to domain union - } -}) +// Filter domain events from a mixed event stream +const events: DurablyEvent[] = getEventStream() +const domainEvents = events.filter(isDomainEvent) + +// Or use with a catch-all listener pattern +function handleEvent(event: DurablyEvent) { + if (isDomainEvent(event)) { + // handle lifecycle events (trigger, complete, fail, cancel, delete, coalesced) + } else { + // handle operational events (leased, progress, step:*, log:write, worker:error) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/api/events.md` around lines 14 - 22, 例示コードでは durably.on('run:fail', ...) 内で isDomainEvent(event) を呼んでいますが "run:fail" は常にドメインイベントなのでそのチェックは不要です。修正案：該当チェックを削除して単純にイベントを処理するコードにするか、より実用的な例に差し替える（全イベント購読例：durably.on('*', handler) 内で isDomainEvent(event) を使ってフィルタする、あるいはイベント配列を filter(isDomainEvent) してから処理するなど）。変更箇所の目印は durably.on(...) 呼び出しと isDomainEvent の使用箇所です。packages/durably/docs/llms.md (1)
356-364: コード例が冗長 —run:completeは常にドメインイベントです
website/api/events.mdと同様に、run:completeリスナー内でのisDomainEvent()チェックは常にtrueになります。一貫性のために両方のドキュメントで例を更新することを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/docs/llms.md` around lines 356 - 364, The example unnecessarily checks isDomainEvent() inside the durably.on('run:complete', ...) handler even though 'run:complete' is always a domain event; remove the import and the conditional so the handler can directly use event.runId. Specifically, update the code that references isDomainEvent and the durably.on listener (the import of isDomainEvent, the if-check, and the branch) to a simple handler that logs event.runId, keeping the durably.on('run:complete', ...) and runId reference.packages/durably/src/job.ts (1)
304-325:settleFromStorageのcancelledステータス処理について321-324行目で
cancelledステータスの処理にreturn文がありません。これは現在の実装では問題ありませんが（関数の最後なので）、将来のコード追加時に意図しない動作を引き起こす可能性があります。♻️ 明示的なreturnを追加する提案
if (run.status === 'cancelled') { cleanup() reject(new CancelledError(runId)) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/src/job.ts` around lines 304 - 325, In settleFromStorage, after handling the 'cancelled' branch where you call cleanup() and reject(new CancelledError(runId)), add an explicit return; i.e., ensure the 'cancelled' case in the settleFromStorage function exits the function just like the other status branches (use the same pattern as the 'completed'/'failed' branches that call cleanup() then return) to avoid accidental fall-through when future code is appended.packages/durably/tests/node/types.test.ts (1)
178-201: 型ガードの戻り値型アサーションについて181行目で
isDomainEventの戻り値型がbooleanとしてテストされていますが、型ガードは通常event is DomainEventという型述語を返します。183-185行目のナローイングテストでは型ガードの動作が正しく検証されていますが、181行目のアサーションは実行時の戻り値型（boolean）をテストしており、TypeScriptの型述語シグネチャではありません。これは意図的かもしれませんが、より正確にするには型述語の振る舞いのみをテストすることを検討してください（183-185行目で既に行われているように）。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/tests/node/types.test.ts` around lines 178 - 201, Replace the incorrect return-type assertion that checks isDomainEvent returns boolean with an assertion that its function signature is a type predicate; specifically, update the test referencing isDomainEvent (the parameter/return assertions around isDomainEvent) to assert the function type equals (e: DurablyEvent) => e is DomainEvent (or simply remove the .returns.toEqualTypeOf<boolean>() line), keeping the existing narrowing check (the if (isDomainEvent(e)) block) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/durably/docs/llms.md`:
- Around line 356-364: The example unnecessarily checks isDomainEvent() inside
the durably.on('run:complete', ...) handler even though 'run:complete' is always
a domain event; remove the import and the conditional so the handler can
directly use event.runId. Specifically, update the code that references
isDomainEvent and the durably.on listener (the import of isDomainEvent, the
if-check, and the branch) to a simple handler that logs event.runId, keeping the
durably.on('run:complete', ...) and runId reference.
In `@packages/durably/src/job.ts`:
- Around line 304-325: In settleFromStorage, after handling the 'cancelled'
branch where you call cleanup() and reject(new CancelledError(runId)), add an
explicit return; i.e., ensure the 'cancelled' case in the settleFromStorage
function exits the function just like the other status branches (use the same
pattern as the 'completed'/'failed' branches that call cleanup() then return) to
avoid accidental fall-through when future code is appended.
In `@packages/durably/tests/node/types.test.ts`:
- Around line 178-201: Replace the incorrect return-type assertion that checks
isDomainEvent returns boolean with an assertion that its function signature is a
type predicate; specifically, update the test referencing isDomainEvent (the
parameter/return assertions around isDomainEvent) to assert the function type
equals (e: DurablyEvent) => e is DomainEvent (or simply remove the
.returns.toEqualTypeOf<boolean>() line), keeping the existing narrowing check
(the if (isDomainEvent(e)) block) intact.
In `@website/api/events.md`:
- Around line 14-22: 例示コードでは durably.on('run:fail', ...) 内で isDomainEvent(event)
を呼んでいますが "run:fail"
は常にドメインイベントなのでそのチェックは不要です。修正案：該当チェックを削除して単純にイベントを処理するコードにするか、より実用的な例に差し替える（全イベント購読例：durably.on('*',
handler) 内で isDomainEvent(event) を使ってフィルタする、あるいはイベント配列を filter(isDomainEvent)
してから処理するなど）。変更箇所の目印は durably.on(...) 呼び出しと isDomainEvent の使用箇所です。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 107893e8-ee3a-403c-abd8-cf3935197986
⛔ Files ignored due to path filters (1)
packages/durably-react/.vitest-attachments/29a9f1aed8057f830dcb6e50abcda0377e0b2912.pngis excluded by!**/*.png
📒 Files selected for processing (19)
examples/server-node-postgres/basic.tsexamples/server-node/basic.tspackages/durably-react/docs/llms.mdpackages/durably/docs/llms.mdpackages/durably/src/durably.tspackages/durably/src/events.tspackages/durably/src/index.tspackages/durably/src/job.tspackages/durably/tests/node/types.test.tspackages/durably/tests/node/wait-for-run.test.tspackages/durably/tests/shared/events.shared.tspackages/durably/tests/shared/run-api.shared.tswebsite/api/create-durably.mdwebsite/api/define-job.mdwebsite/api/durably-react/types.mdwebsite/api/events.mdwebsite/api/index.mdwebsite/guide/server-mode.mdwebsite/public/llms.txt
coji
changed the title
- Fix isDomainEvent() examples in docs to show practical usage with DurablyEvent type instead of redundant check inside typed handler - Add explicit return after cancelled case in settleFromStorage - Remove redundant boolean return type assertion for isDomainEvent type predicate (narrowing test already covers this) - Regenerate llms.txt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/durably/src/job.ts (1)
12-13:1000の既定値は 1 箇所に寄せたいです。
packages/durably/src/durably.ts:87-92にも同じ既定値があるので、ここに別定数を置くと片方だけ更新されたときにcreateDurably()経由とwaitForRunCompletion()直呼び出しで挙動がズレます。共有定数を切り出すか、この helper には常に解決済みの値を渡す形にしておく方が安全です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/src/job.ts` around lines 12 - 13, DEFAULT_WAIT_POLLING_INTERVAL_MS is duplicated (also defined in createDurably) causing divergent behavior; unify the value by exporting a single shared constant and using it everywhere or by changing callers to pass an already-resolved pollingIntervalMs into waitForRunCompletion. Concretely, remove the local DEFAULT_WAIT_POLLING_INTERVAL_MS, add a single exported constant (e.g., WAIT_POLLING_INTERVAL_MS) in a shared module and update createDurably and waitForRunCompletion to import and use that constant (or alter waitForRunCompletion to require pollingIntervalMs as an argument and update createDurably to pass its resolved value).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/durably/src/job.ts`:
- Around line 111-115: Validate the pollingIntervalMs option (ensure it's a
positive, finite number and fall back to a safe default or throw) before passing
it to setInterval; do not call setInterval with 0, negative, NaN, or Infinity.
Add an in-flight guard (e.g., a pollInFlight boolean) around the getRun()
polling call: check pollInFlight and skip starting a new getRun() if true, set
pollInFlight = true before awaiting getRun(), and reset pollInFlight = false in
a finally block to guarantee cleanup; update references in the polling logic
that currently call setInterval(...) and invoke getRun() so they use this
validation and the pollInFlight guard.
---
Nitpick comments:
In `@packages/durably/src/job.ts`:
- Around line 12-13: DEFAULT_WAIT_POLLING_INTERVAL_MS is duplicated (also
defined in createDurably) causing divergent behavior; unify the value by
exporting a single shared constant and using it everywhere or by changing
callers to pass an already-resolved pollingIntervalMs into waitForRunCompletion.
Concretely, remove the local DEFAULT_WAIT_POLLING_INTERVAL_MS, add a single
exported constant (e.g., WAIT_POLLING_INTERVAL_MS) in a shared module and update
createDurably and waitForRunCompletion to import and use that constant (or alter
waitForRunCompletion to require pollingIntervalMs as an argument and update
createDurably to pass its resolved value).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d144caa-9f77-4add-8b12-1b92a3e15a77
📒 Files selected for processing (5)
packages/durably/docs/llms.mdpackages/durably/src/job.tspackages/durably/tests/node/types.test.tswebsite/api/events.mdwebsite/public/llms.txt
✅ Files skipped from review due to trivial changes (1)
- packages/durably/docs/llms.md
🚧 Files skipped from review as they are similar to previous changes (3)
- website/api/events.md
- packages/durably/tests/node/types.test.ts
- website/public/llms.txt
- Validate pollingIntervalMs is a positive finite number - Add pollInFlight guard to prevent concurrent getRun() requests when storage responses are slower than the polling interval Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
coji
added a commit
that referenced
this pull request
Mar 28, 2026
- Add step 5 to spec-review: check for invalid input tests on new public API options, async in-flight guards, doc example correctness, and explicit behavior preservation criteria - Add matching items to spec-draft quality checklist - Remove remaining vitest-attachments PNG Learned from #169 CodeRabbit review: pollingIntervalMs lacked input validation and in-flight guard, isDomainEvent examples were redundant. These should have been caught at spec stage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
coji
added a commit
that referenced
this pull request
Mar 28, 2026
…171) - Add step 5 to spec-review: check for invalid input tests on new public API options, async in-flight guards, doc example correctness, and explicit behavior preservation criteria - Add matching items to spec-draft quality checklist - Remove remaining vitest-attachments PNG Learned from #169 CodeRabbit review: pollingIntervalMs lacked input validation and in-flight guard, isDomainEvent examples were redundant. These should have been caught at spec stage. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 29, 2026
Merged
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.
Summary
isDomainEvent()type guardwaitForRunCompletion()for cross-process observation (feat: polling fallback for waitForRun in multi-worker setups #152) — when no in-process event fires, pollsstore.getRun()at configurable interval (default:pollingIntervalMsfromcreateDurably()config, default 1000ms)Changes
events.ts,index.tsDomainEvent,OperationalEventtypes,isDomainEvent()guardjob.ts,durably.tspollingIntervalMsoption inWaitForRunOptionsandDurablyOptionswait-for-run.test.ts(new),events.shared.ts,run-api.shared.ts,types.test.tsllms.md,website/api/,website/guide/, examplesContext
Step 2/4 of #162 (core internal restructuring). Supersedes #152.
Test plan
pnpm validatepasses (format, lint, typecheck, tests)wait-for-run.test.tscovers: cross-process detection via polling, same-process fast path, timeout with polling active, custom intervalisDomainEvent()for all 16 event types🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
新機能
waitForRun()とtriggerAndWait()に、カスタマイズ可能なストレージポーリング間隔オプションを追加。ドキュメント
テスト