Skip to content

Handle session.canvas.closed by removing from open_canvases snapshot#1604

Merged
jmoseley merged 1 commit into
mainfrom
jmoseley/handle-canvas-closed-snapshot
Jun 9, 2026
Merged

Handle session.canvas.closed by removing from open_canvases snapshot#1604
jmoseley merged 1 commit into
mainfrom
jmoseley/handle-canvas-closed-snapshot

Conversation

@jmoseley

@jmoseley jmoseley commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What

Completes the symmetric pair with session.canvas.opened: the SDKs now remove a canvas from their in-memory open_canvases snapshot when the runtime emits session.canvas.closed.

Why

The runtime now emits a symmetric session.canvas.closed ephemeral event (payload { instanceId, extensionId, canvasId }, instanceId required + non-empty) when a canvas is genuinely removed — mirroring the existing session.canvas.opened (copilot-agent-runtime #9489, shipped in CLI 1.0.60).

The generated event types already landed via codegen, but the hand-written handlers only ever upserted on open, so the snapshot was append-only and never shrank on close. This left consumers' open-canvas state stale.

The consumer driving this is github-app's "sticky terminal canvas" fix: a right-panel canvas the user closes resurrects on warm session-reselect. That fix needs the SDK snapshot to shrink on close so reselect stops resurrecting closed canvases.

How

Adds a symmetric removal handler in every SDK that maintains the open-canvases snapshot — Rust, Node, Python, Go, .NET. Each:

  • removes the matching instanceId on session.canvas.closed, acquiring the same lock the upsert path uses;
  • warns and no-ops on an empty/missing instanceId;
  • is idempotent (closing an absent instanceId is a no-op);
  • leaves the resume path untouched and does not treat opened/stale events as removals (only the explicit closed event removes — provider unregister still marks instances stale via a re-emitted opened, which continues to upsert).

Generated files were not hand-edited; the change is purely in the hand-written session handlers + unit tests.

Java — intentionally excluded

Java has no open-canvases snapshot feature in java/src/main at all (no accessor, no opened-upsert handler, no resume capture) — only the generated event types exist. There is nothing symmetric to add; covering it would mean building the entire feature net-new, which is out of scope for this focused bug fix.

Tests

New unit tests in each language open two canvases, close one (assert the closed instance is gone and the other remains), and cover idempotency (closing an absent id) and the empty-instanceId no-op.

  • Rust: cargo test --features test-support --test session_test — pass
  • Node: vitest run test/client.test.ts — 95 pass
  • Python: pytest test_canvas.py — 12 pass
  • Go: go test . — pass
  • .NET: dotnet test --filter CanvasTests — 10 pass

The runtime now emits a symmetric `session.canvas.closed` ephemeral event
(payload `{ instanceId, extensionId, canvasId }`) when a canvas is genuinely
removed, mirroring the existing `session.canvas.opened`
(copilot-agent-runtime #9489, shipped in CLI 1.0.60). The generated event types
already landed via codegen, but the hand-written handlers only ever upserted on
open, so the in-memory `open_canvases` snapshot was append-only and never shrank
on close.

This adds the symmetric removal handler in every SDK that maintains the
open-canvases snapshot: Rust, Node, Python, Go, and .NET. Each:
- removes the matching instanceId from the snapshot on `session.canvas.closed`,
  acquiring the same lock the upsert path uses;
- warns and no-ops on an empty/missing instanceId;
- is idempotent (closing an absent instanceId is a no-op);
- leaves the resume path untouched and does not treat opened/stale events as
  removals (only the explicit closed event removes).

Java is intentionally excluded: it has no open-canvases snapshot feature at all
(only the generated event types exist), so there is nothing symmetric to add.

The consumer is github-app's "sticky terminal canvas" fix, which needs the SDK
snapshot to shrink on close so warm session-reselect stops resurrecting closed
canvases.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jmoseley jmoseley requested a review from a team as a code owner June 9, 2026 03:31
Copilot AI review requested due to automatic review settings June 9, 2026 03:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR completes the open-canvases snapshot lifecycle across the SDKs by removing entries from the in-memory open_canvases/openCanvases snapshot when a session.canvas.closed event is received, keeping consumer state aligned with the runtime’s authoritative “open canvas instances” view.

Changes:

  • Add session.canvas.closed handling to remove the matching instanceId from the open-canvases snapshot in Rust, Node, Python, Go, and .NET.
  • Update snapshot documentation/comments to reflect that both opened and closed events affect the snapshot.
  • Add symmetric unit tests in each language to cover removal, idempotency, and empty/missing instanceId no-op behavior.
Show a summary per file
File Description
rust/src/session.rs Handles session.canvas.closed by removing the matching instance from the open_canvases snapshot.
rust/tests/session_test.rs Adds coverage for close-driven removal, idempotency, and empty-instance no-op behavior.
python/copilot/session.py Removes closed canvas instances from _open_canvases snapshot and updates docstring accordingly.
python/test_canvas.py Adds unit test validating removal, idempotency, and warning/no-op behavior on empty instance id.
nodejs/src/session.ts Dispatches session.canvas.closed to a new removal path that updates the openCanvasInstances snapshot.
nodejs/test/client.test.ts Adds a unit test for close-driven snapshot removal and invalid payload warning behavior.
go/session.go Extends event-driven snapshot updates to handle close events via removal under the same mutex.
go/session_test.go Adds a unit test verifying close removes one instance, idempotency, and invalid close no-op behavior.
dotnet/src/Session.cs Updates event processing to remove closed instances from OpenCanvases snapshot and updates remarks.
dotnet/test/Unit/CanvasTests.cs Adds unit test verifying close removal, idempotency, and empty instance id no-op behavior.

Copilot's findings

  • Files reviewed: 10/10 changed files
  • Comments generated: 0

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR adds the symmetric session.canvas.closed removal handler to all five SDKs that maintain the open-canvases snapshot: Node.js, Python, Go, .NET, and Rust.

Consistency across modified SDKs

All five implementations are symmetric with their existing session.canvas.opened upsert path:

Concern Node.js Python Go .NET Rust
Removal logic Array.filter list comp iterate + rebuild LINQ Where Vec::retain
Empty instanceId → warn + no-op
Idempotent (absent id → no-op)
Same lock as upsert path N/A (single-threaded) _open_canvases_lock openCanvasesMu.Lock() (no lock, consistent with upsert) open_canvases.write()
Warning message console.warn logger.warning fmt.Printf _logger.LogWarning warn!
Tests

Warning messages all use the same text: "failed to deserialize session.canvas.closed payload", consistent with the existing opened-path wording.

Java exclusion

Confirmed: java/src/main has no open-canvases snapshot feature — no openCanvases accessor, no session.canvas.opened upsert handler, and no resume capture. Only the generated event types (SessionCanvasClosedEvent, OpenCanvasInstance, etc.) exist. The exclusion is correctly documented in the PR description and is the right call for a focused bug fix.

No cross-SDK consistency issues found.

Generated by SDK Consistency Review Agent for issue #1604 · sonnet46 1.1M ·

@stephentoub stephentoub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did we not add these APIs to Java?

@jmoseley

jmoseley commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Did we not add these APIs to Java?

When they were originally added, the Java implementation was missing a bunch of stuff. I can kick off a session to build this into the Java client as well.

@jmoseley jmoseley added this pull request to the merge queue Jun 9, 2026
Merged via the queue into main with commit 391bcd4 Jun 9, 2026
39 checks passed
@jmoseley jmoseley deleted the jmoseley/handle-canvas-closed-snapshot branch June 9, 2026 04:09
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.

3 participants