Skip to content

EW-10817 Add HibernationManager unit tests#6732

Open
harrishancock wants to merge 1 commit into
mainfrom
harris/EW-10817-hibernation-manager-tests
Open

EW-10817 Add HibernationManager unit tests#6732
harrishancock wants to merge 1 commit into
mainfrom
harris/EW-10817-hibernation-manager-tests

Conversation

@harrishancock
Copy link
Copy Markdown
Collaborator

Adds a kj_test suite covering normal HibernationManager behavior (basic comm, close, binary, auto-response, multi-IR, multi-WS, hibernation flows, output gate interactions) along with regression tests for EW-10817 that use KJ_EXPECT_LOG to capture the production "another message send is already in progress" assertion. The regression tests pass while the bug exists and fail loudly when the fix lands. Code comments document each test's purpose, lifecycle, and known workarounds.

Supporting TestFixture additions: SetupParams::actorLoopback so the actor and the HibernationManager share one Loopback; newIoContext() and newIncomingRequest(IoContext&) for multiple IRs per IoContext; drainAndDestroy() and pollEventLoop() helpers; getActor() and getTimerChannel() accessors so tests can construct the HM outside any IoContext; resetActor() to simulate eviction by reconstructing the Worker::Actor with the same id and Loopback.

Two workarounds for known bugs are noted at their use sites: a leaked api::WebSocket ref to dodge the AsyncObject destructor issue, and an explicit end1->receive() drain in some EW-10817 repros to consume the orphaned BlockedSend at teardown. Both go away once EW-10817 is fixed.

Assisted-by: OpenCode:claude-opus-4.7

@harrishancock harrishancock requested review from a team as code owners May 6, 2026 10:43
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 6, 2026

That's correct. The alloc<T>(args...) passes env.js plus the websocket as constructor arguments. The WebSocket constructor takes (jsg::Lock&, kj::Own<kj::WebSocket>). This is the standard pattern.

After careful review, I believe this is a high-quality PR. The tests are well-structured, well-commented, and the TestFixture additions are properly designed. The intentional leaks and workarounds are clearly documented with TODO references to the bug being tracked. The BUILD.bazel changes are correct.

The only potential issue I see is minor: the enterContext method constructs an Environment without initializing the features field, but this is a pre-existing pattern (also present in runInIoContext) and the HM tests don't use features, so it's not actionable for this PR.

LGTM

github run

@harrishancock harrishancock force-pushed the harris/EW-10817-hibernation-manager-tests branch 2 times, most recently from a07592b to fd5b913 Compare May 6, 2026 14:20
Comment thread src/workerd/tests/test-fixture.h Outdated
Comment thread src/workerd/tests/test-fixture.h
Comment thread src/workerd/io/hibernation-manager-test.c++
Comment thread src/workerd/io/hibernation-manager-test.c++ Outdated
Comment thread src/workerd/io/hibernation-manager-test.c++ Outdated
Copy link
Copy Markdown
Contributor

@jclee jclee left a comment

Choose a reason for hiding this comment

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

Looks good...

It's great to have unit test coverage for this, thanks! I guess we'd been relying mostly on integration tests for coverage, but of course that makes it very hard to test precise event orderings like this.

@harrishancock harrishancock force-pushed the harris/EW-10817-hibernation-manager-tests branch from fd5b913 to b966caa Compare May 11, 2026 14:24
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 11, 2026

Merging this PR will improve performance by 14.95%

⚡ 1 improved benchmark
✅ 71 untouched benchmarks
⏩ 129 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
Encode_ASCII_256[TextEncoder][0/0/256] 3.1 ms 2.7 ms +14.95%

Comparing harris/EW-10817-hibernation-manager-tests (9b82143) with main (ac31559)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Adds a kj_test suite covering normal HibernationManager behavior (basic
comm, close, binary, auto-response, multi-IR, multi-WS, hibernation flows,
output gate interactions) along with regression tests for EW-10817 that use
KJ_EXPECT_LOG to capture the production "another message send is already in
progress" assertion. The regression tests pass while the bug exists and
fail loudly when the fix lands. Code comments document each test's purpose,
lifecycle, and known workarounds.

Supporting TestFixture additions: SetupParams::actorLoopback so the actor
and the HibernationManager share one Loopback; newIoContext() and
newIncomingRequest(IoContext&) for multiple IRs per IoContext;
drainAndDestroy() and pollEventLoop() helpers; getActor() and
getTimerChannel() accessors so tests can construct the HM outside any
IoContext; resetActor() to simulate eviction by reconstructing the
Worker::Actor with the same id and Loopback.

Two workarounds for known bugs are noted at their use sites: a leaked
api::WebSocket ref to dodge the AsyncObject destructor issue, and an
explicit end1->receive() drain in some EW-10817 repros to consume the
orphaned BlockedSend at teardown. Both go away once EW-10817 is fixed.

Assisted-by: OpenCode:claude-opus-4.7
@harrishancock harrishancock force-pushed the harris/EW-10817-hibernation-manager-tests branch from b966caa to 9b82143 Compare May 11, 2026 15:18
@harrishancock
Copy link
Copy Markdown
Collaborator Author

Added one new test case in the latest commit.

Unfortunately, I failed to push an update between addressing review comments and rebasing on main. :(

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