Skip to content

feat: feat(cluster-protocol): add the Rust WebSocket binding and cloud data-plane contract#797

Closed
tomdps wants to merge 1 commit into
devfrom
zeroshot/cobalt-cipher-48
Closed

feat: feat(cluster-protocol): add the Rust WebSocket binding and cloud data-plane contract#797
tomdps wants to merge 1 commit into
devfrom
zeroshot/cobalt-cipher-48

Conversation

@tomdps

@tomdps tomdps commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Closes #651

@tomdps
tomdps enabled auto-merge July 25, 2026 02:30
@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR makes WebSocket passthrough cancellation registration deterministic and adds cancellation lifecycle regression coverage.

  • Introduces a readiness gate so passthrough work starts only after its abort handle is registered.
  • Adds a drop guard to release request IDs and cancellation entries on completion, cancellation, or panic.
  • Adds unit and integration tests for fast completion, cancellation cleanup, and request-ID reuse.

Confidence Score: 3/5

This PR should not merge until cleanup for an old request is prevented from deleting the cancellation handle of a newer request with the same ID.

The separate admission and cancellation registry removals create an ownership race in which request-ID reuse can leave an active retry without a usable cancellation handle.

Files Needing Attention: crates/openengine-cluster-server/src/websocket.rs

Important Files Changed

Filename Overview
crates/openengine-cluster-server/src/websocket.rs Adds gated task startup and unconditional cleanup, but cleanup can remove a newer same-ID request's cancellation handle.
crates/openengine-cluster-server/src/websocket/tests.rs Adds deterministic unit coverage for the original spawn-before-registration stale-entry race.
crates/openengine-cluster-server/tests/websocket.rs Adds integration coverage for cancellation cleanup, ID reuse, and rapid completion behavior, but not the cross-task cleanup race.

Reviews (1): Last reviewed commit: "feat: implement #651 - feat(cluster-prot..." | Re-trigger Greptile

Comment on lines +297 to +298
self.state.in_flight_ids.lock().remove(&self.id);
self.cancel_registry.lock().remove(&self.id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Cleanup removes the retry handle

When a cancelled or completed request is immediately retried with the same ID, the old guard releases in_flight_ids before separately removing from cancel_registry, allowing the retry's newly registered abort handle to be removed and causing a later $/cancelRequest to be silently ignored.

@tomdps
tomdps added this pull request to the merge queue Jul 25, 2026
@tomdps
tomdps removed this pull request from the merge queue due to a manual request Jul 25, 2026
@tomdps

tomdps commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

Post-validation review found an additional P1 ownership race: the old request guard releases in_flight_ids and then removes cancel_registry separately, so an immediate same-ID retry can register between those operations and have its new cancellation handle removed by the old guard. This pull request was removed from the merge queue and is superseded by a fresh #651 hardening run whose contract requires generation/ownership-safe cleanup.

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