Skip to content

fix(node): do not reclaim READY SAB slots in beginFrame#214

Merged
RtlZeroMemory merged 3 commits intomainfrom
fix/pr213-beginframe-ready-slot
Feb 26, 2026
Merged

fix(node): do not reclaim READY SAB slots in beginFrame#214
RtlZeroMemory merged 3 commits intomainfrom
fix/pr213-beginframe-ready-slot

Conversation

@RtlZeroMemory
Copy link
Owner

@RtlZeroMemory RtlZeroMemory commented Feb 26, 2026

Summary

Why

Reclaiming a READY slot in beginFrame() creates a window where the worker can still observe old mailbox metadata but find slot state changed to WRITING with the same token, which can escalate to a backend fatal.

Testing

  • npm run build -- --pretty false
  • node --test --test-concurrency=1 packages/node/dist/__tests__/worker_integration.test.js

Summary by CodeRabbit

  • Bug Fixes

    • Prevents unintended slot reclamation during high-pressure frame submission, reducing contention under load.
    • Ensures oversized bracketed-paste is only treated as a paste event when an explicit oversized marker is present.
  • Tests

    • Adds integration tests verifying SAB-based frame submission behavior under backpressure.
    • Adds tests validating marker-based oversized-paste detection.

@coderabbitai
Copy link

coderabbitai bot commented Feb 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c97234e and 6f293bf.

📒 Files selected for processing (1)
  • packages/node/src/__e2e__/terminal_io_contract.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/node/src/e2e/terminal_io_contract.e2e.test.ts

📝 Walkthrough

Walkthrough

Adds an option to SAB slot acquisition to prevent reclaiming READY slots under pressure and updates Node backend calls to use it; also adds a unit test exercising the new behavior and adjusts an e2e test's oversized-paste detection.

Changes

Cohort / File(s) Summary
Unit Test
packages/node/src/__tests__/worker_integration.test.ts
New test "backend: SAB beginFrame does not reclaim READY slot under pressure" exercising SAB-based frame submission with backpressure and single-slot configuration.
Backend SAB Acquisition Logic
packages/node/src/backend/nodeBackend.ts
acquireSabSlot gains an optional opts: { allowReadyReclaim?: boolean } (default true). When allowReadyReclaim is false and no free slot exists, the function returns -1 instead of reclaiming READY slots. Calls in beginFrame and initial SAB acquisition now pass { allowReadyReclaim: false }.
E2E Test (paste handling)
packages/node/src/__e2e__/terminal_io_contract.e2e.test.ts
Changed oversized paste detection to use a marker-prefixed payload and a pre-collection pass; final assertion now checks for paste events containing the marker rather than asserting no paste events at all.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐇 I hopped through queues and counted each slot,
Kept READY untouched when the pressure was hot.
A gentle refrain, no snatch in the night,
I waited my turn till the frame came to light.
🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'fix(node): do not reclaim READY SAB slots in beginFrame' accurately and specifically describes the main change: disabling READY slot reclamation in the beginFrame function.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/pr213-beginframe-ready-slot

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/node/src/__e2e__/terminal_io_contract.e2e.test.ts`:
- Around line 896-904: The test block around
oversizedPaste/findIndex/assert.equal is failing formatting; run the project's
formatter (Prettier) or lint autofix to reformat this snippet so it matches
project style (e.g., run npm run lint -- --fix or the repo's formatter) and
commit the formatted changes; focus on the lines referencing oversizedEvents,
findIndex, textDecoder.decode(ev.bytes).includes(oversizedMarker),
oversizedPaste, and the assert.equal call to ensure spacing/line breaks match
project formatting rules.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e0d34a and c97234e.

📒 Files selected for processing (1)
  • packages/node/src/__e2e__/terminal_io_contract.e2e.test.ts

@RtlZeroMemory RtlZeroMemory merged commit 429fe15 into main Feb 26, 2026
28 checks passed
@RtlZeroMemory RtlZeroMemory deleted the fix/pr213-beginframe-ready-slot branch February 28, 2026 04:19
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