Skip to content

Conversation

myftija
Copy link
Member

@myftija myftija commented Sep 26, 2025

The completeWaitpoint method ignores Prisma update errors of type P2025 to
handle concurrent attempts to complete a waitpoint token. We have an
error hook on the Prisma client which logs these errors though.

This PR switches to updateMany which doesn't throw an error if the
waitpoint is in an unexpected state. The behavior remains otherwise
unchanged. We currently continue even if the waitpoint is not in PENDING,
which could lead to race conditions in certain corner cases, though very
unlikely. We added a log line to see how often concurrent completion
attempts happen.

The `completeWaitpoint` method ignores Prisma update errors of type `P2025` to
handle concurrent attempts to complete a waitpoint token. We have an
error hook on the Prisma client which logs these errors though.

This PR switches to `updateMany` which doesn't throw an error if the
waitpoint is in an unexpected state. The behavior remains otherwise
unchanged. We currently continue even if the waitpoint is not in `PENDING`,
which could lead to race conditions in certain corner cases, though very
unlikely. We added a log line to see how often concurrent completion
attempts happen.
Copy link

changeset-bot bot commented Sep 26, 2025

⚠️ No Changeset found

Latest commit: c969f50

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

Walkthrough

The system modifies waitpoint completion logic to use an updateMany operation instead of a single-record update. Variable names are updated accordingly. Error handling is simplified: any update error is logged and rethrown; the previous P2025-specific handling is removed. If updateResult.count is 0, it logs that a non-pending waitpoint was targeted. The code then fetches the waitpoint by id in all cases and retains validations for existence and completed status. Error messages are generalized to "Waitpoint not found" and "Waitpoint not completed," and control flow now relies on updateMany results.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description does not follow the provided template because it lacks the issue closure line, the checklist, testing steps, a changelog section, and any placeholder for screenshots. Please update the description to include “Closes #”, complete the checklist items, add a Testing section describing how you verified the change, include a Changelog summary, and attach any relevant screenshots according to the repository template.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title follows the repository’s conventional commit style and clearly summarizes the main change of fixing misleading error logs in the run-engine waitpoint update path without extraneous detail.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-waitpoint-update-error-logs

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 05b6a26 and c969f50.

📒 Files selected for processing (1)
  • internal-packages/run-engine/src/engine/systems/waitpointSystem.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
🧬 Code graph analysis (1)
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts (1)
packages/core/src/utils.ts (1)
  • tryCatch (5-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts (1)

82-106: Solid concurrency-safe update strategy

Switching to updateMany removes the noisy P2025 path while keeping the post-checks intact, and the new info-level log gives us useful signal on concurrent completions without surfacing as errors. 👍


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@myftija myftija merged commit 3ceea77 into main Sep 26, 2025
31 checks passed
@myftija myftija deleted the fix-waitpoint-update-error-logs branch September 26, 2025 19:16
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