Skip to content

fix(cli): isolate concurrent session shares - #57

Merged
Raghav1428 merged 1 commit into
mainfrom
fix/session-share-isolation
Sep 1, 2026
Merged

Raghav1428 merged 1 commit into
mainfrom
fix/session-share-isolation

Conversation

@Raghav1428

@Raghav1428 Raghav1428 commented Sep 1, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

shareSession() wrote both exports to fixed paths in os.tmpdir() — session.jsonl and session.html. Two /share invocations overlapping (two sessions, or one started before the previous upload finished) meant the second clobbered the first's export, and whichever finished first deleted the file the other was still uploading. The result was a share containing the wrong session, or a failed upload.

Each invocation now gets its own mkdtempSync directory and the finally block removes only that directory. Creating the directory is guarded, so an unwritable os.tmpdir() shows the same Failed to export session message the old code produced rather than rejecting out of shareSession.

Ported from upstream pi, with the temp prefix renamed to knightcode-share-.

Testing

  • New test/session-share.test.ts interleaves two shares deterministically and asserts each upload carries its own session's content.
  • Verified the test fails on main (expected [ 'B' ] to deeply equal [ 'A', 'B' ]) and passes with the fix.
  • bun run check-types clean.

@vercel

vercel Bot commented Sep 1, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
knightcode Ready Ready Preview Sep 1, 2026 5:48pm UTC

@cubic-dev-ai cubic-dev-ai Bot 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.

No issues found across 3 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would auto-approve. Fixes a concurrency bug where overlapping session shares overwrote each other's temp files by giving each invocation its own temp directory, with a test verifying isolation.

Re-trigger cubic

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 3 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread packages/cli/src/modes/interactive/session-share.ts Outdated
Both exports were written to fixed paths in os.tmpdir(), so a second /share
overwrote the first one's session.jsonl and session.html, and whichever
finished first deleted the file the other was still uploading — producing a
share of the wrong session, or a failed upload.

Give each invocation its own mkdtemp directory and remove only that. A
directory that cannot be created still surfaces the usual export error
instead of rejecting out of shareSession.

Ported from upstream pi.
@Raghav1428
Raghav1428 force-pushed the fix/session-share-isolation branch from 9fa58de to 13586e1 Compare September 1, 2026 17:47
@Raghav1428

Copy link
Copy Markdown
Collaborator Author

Addressed — mkdtempSync is now wrapped so a creation failure shows the usual Failed to export session message and returns, instead of rejecting out of shareSession.

@cubic-dev-ai cubic-dev-ai Bot 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.

0 issues found across 3 files (changes from recent commits).

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would auto-approve. Fixes a concurrency bug in session sharing by isolating each share's temp directory, with a deterministic test verifying isolation.

Re-trigger cubic

@Raghav1428
Raghav1428 merged commit 71b3a41 into main Sep 1, 2026
4 checks passed

This branch was successfully deployed

1 active deployment
Preview — 13586e10 Deployed Sep 1, 2026 by vercel[bot]
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