Skip to content

test(rivetkit): cover async websocket close db access#4938

Merged
NathanFlurry merged 1 commit intomainfrom
05-04-test_rivetkit_websocket_close_db
May 5, 2026
Merged

test(rivetkit): cover async websocket close db access#4938
NathanFlurry merged 1 commit intomainfrom
05-04-test_rivetkit_websocket_close_db

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented May 4, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 4, 2026

Code Review

Overview

This PR re-enables three previously skipped tests (TODO #4705) that verify async WebSocket close handler DB access works correctly during sleep shutdown. The changes are a meaningful quality improvement.


What Changed

  1. Three test.skip calls converted to active tests.
  2. Actor names switched to crypto.randomUUID() suffixes for isolation.
  3. waitForAction(actor.getStatus, ...) polling replaced with time-based waitFor(...).
  4. Richer assertions: checks event ordering (close-start -> close-finish -> sleep -> wake) instead of just counting wake/sleep cycles.

Positive Observations

Polling removal is the right fix. The previous approach called actor.getStatus (an actor action) inside a polling loop to wait for sleep to complete. Per project guidelines, each action call counts as activity and can reset the sleep deadline, making those tests inherently self-defeating. Switching to a time-based waitFor is the correct resolution.

UUID-based actor names correctly prevent cross-test actor identity collisions, which was likely a source of the original flakiness.

Event ordering assertions are semantically stronger than the old sleepCount/startCount checks. Verifying close-start < close-finish < sleep < lastIndexOf("wake") directly encodes the contract being tested.

lastIndexOf("wake") is the right call - onWake fires both at initial startup and after sleep resumes, so there are two "wake" entries and the test correctly targets the second one.


Issues

Time-based waits with no event signal. The waitFor(driverTestConfig, RAW_WS_HANDLER_DELAY + RAW_WS_HANDLER_SLEEP_TIMEOUT + 1_000) calls (1350 ms total) are a pure wall-clock sleep with no event that fires when the sequence completes. This works because the budget is generous relative to the delays, but it is inherently load-sensitive. On an overloaded CI runner, the sleep completion and DB write could land after the waitFor expires, causing a spurious failure. A comment explaining the trade-off (polling was avoided because each action resets the sleep deadline) would help future maintainers.

Duplicate test body. The two test cases (onclose property vs addEventListener("close", ...)) are structurally identical apart from the actor name. If the ordering assertions or wait duration change, both need to be updated in sync. A shared helper accepting the actor handle would keep them DRY.

No comment on waitFor explaining why time-based. The check:wait-for-comments script only enforces comments on vi.waitFor, not the project's waitFor helper. Given how non-obvious the rationale is, a short comment above each waitFor call would prevent future maintainers from reverting to the polling pattern.


Minor Nits

  • The removed sleepCount === 1 / startCount === 2 assertions are replaced by event ordering checks, which is strictly stronger for this use case. The coverage shift is intentional and correct.
  • The 30,000 ms test timeout is appropriate for the 1350 ms wait plus real-timer overhead headroom.

Summary

The core approach is sound and the tests are now functionally correct. The main risk is latent time-sensitivity in the waitFor calls - a comment explaining why polling was avoided would protect against accidental regression.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Preview packages published to npm

Install with:

npm install rivetkit@pr-4938

All packages published as 0.0.0-pr.4938.50d75f0 with tag pr-4938.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-50d75f0
docker pull rivetdev/engine:full-50d75f0
Individual packages
npm install rivetkit@pr-4938
npm install @rivetkit/react@pr-4938
npm install @rivetkit/rivetkit-napi@pr-4938
npm install @rivetkit/workflow-engine@pr-4938

@NathanFlurry NathanFlurry changed the base branch from 05-04-fix_rivetkit_keepawake_sleep_grace to graphite-base/4938 May 4, 2026 13:58
@NathanFlurry NathanFlurry force-pushed the graphite-base/4938 branch from e887f7e to d3d9f0e Compare May 4, 2026 15:36
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4938 to 05-04-fix_rivetkit_keepawake_sleep_grace May 4, 2026 15:36
@NathanFlurry NathanFlurry force-pushed the 05-04-test_rivetkit_websocket_close_db branch from e37a5c2 to a35a0a4 Compare May 4, 2026 15:36
Base automatically changed from 05-04-fix_rivetkit_keepawake_sleep_grace to main May 5, 2026 14:57
@NathanFlurry NathanFlurry merged commit a35a0a4 into main May 5, 2026
22 of 28 checks passed
@NathanFlurry NathanFlurry deleted the 05-04-test_rivetkit_websocket_close_db branch May 5, 2026 14:58
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.

1 participant