Skip to content

Conversation

@OneBlue
Copy link
Collaborator

@OneBlue OneBlue commented Jan 9, 2026

Summary of the Pull Request

This will be needed for the CLI to open & maintain its session across invocations

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed


// Session managment.
HRESULT CreateSession([in] const struct WSLA_SESSION_SETTINGS* Settings, [out]IWSLASession** Session);
HRESULT CreateSession([in] const struct WSLA_SESSION_SETTINGS* Settings, WSLASessionFlags Flags, [out]IWSLASession** Session);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kevpar: This is one of the API changes that I mentioned. This will allow a caller to specify that a session should outlive its last COM client reference.

The WSLASessionFlagsOpenExisting flag will make it easy for the CLI to have a "create-or-open" logic when setting up its COM session

@benhillis
Copy link
Member

For persistent sessions what are you thinking for lifetime of the VM? We likely want to clean it up after some period of inactivity.

@OneBlue
Copy link
Collaborator Author

OneBlue commented Jan 9, 2026

For persistent sessions what are you thinking for lifetime of the VM? We likely want to clean it up after some period of inactivity.

This is something that we still need to design. Most likely we're going to need some form of configuration file for the container CLI, and in it we can have an inactivity timeout, similar to what we do in WSL

@OneBlue OneBlue marked this pull request as ready for review January 10, 2026 19:01
@OneBlue OneBlue requested a review from a team as a code owner January 10, 2026 19:01
Copilot AI review requested due to automatic review settings January 10, 2026 19:01
Copy link
Contributor

Copilot AI left a comment

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 adds API flags for creating and opening persistent WSL sessions that can outlive their COM reference lifetime. The main purpose is to support the CLI's ability to maintain sessions across invocations by introducing WSLASessionFlagsPersistent and WSLASessionFlagsOpenExisting flags.

Changes:

  • Added session flag enums (WSLASessionFlagsPersistent, WSLASessionFlagsOpenExisting) to control session lifecycle behavior
  • Removed the deprecated IWSLAVirtualMachine interface and migrated its functionality to IWSLASession
  • Replaced Shutdown() with Terminate() and changed WSLAVirtualMachine from a COM object to an internal std::optional member
  • Added comprehensive test coverage for persistent session behavior including creation, reopening, and termination

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/windows/wslaservice/inc/wslaservice.idl Added WSLASessionFlags enum, updated CreateSession signature, removed IWSLAVirtualMachine interface, renamed Shutdown to Terminate, added testing methods to IWSLASession
src/windows/wslaservice/exe/WSLAUserSession.h Added m_persistentSessions vector to store strong references, updated CreateSession signature, added logic to remove terminated persistent sessions
src/windows/wslaservice/exe/WSLAUserSession.cpp Implemented session lookup by name with OpenExisting flag support, added persistent session reference management
src/windows/wslaservice/exe/WSLASession.h Changed m_virtualMachine from ComPtr to std::optional, renamed Shutdown to Terminate, added Terminated() method, moved VM methods from IWSLAVirtualMachine
src/windows/wslaservice/exe/WSLASession.cpp Refactored Terminate() to be callable explicitly, moved VM operation wrappers from deleted interface, changed VM initialization to use emplace
src/windows/wslaservice/exe/WSLAVirtualMachine.h Removed COM interface inheritance, changed methods from HRESULT-returning to void/exception-throwing
src/windows/wslaservice/exe/WSLAVirtualMachine.cpp Removed WaitPid, Shutdown methods, changed MapPort/Unmount/Signal to throw exceptions instead of returning HRESULT
src/windows/wslaservice/exe/WSLAContainer.cpp Updated MapPort calls to use try/catch instead of LOG_IF_FAILED since it now throws
src/windows/wslaservice/exe/WSLAProcessControl.cpp Updated Signal call to handle void return type
src/windows/common/WslClient.cpp Updated CreateSession call to pass WSLASessionFlagsNone, removed IWSLAVirtualMachine usage
test/windows/WSLATests.cpp Updated all tests to use new API, replaced Shutdown with session.reset() or Terminate(), added PersistentSession test with comprehensive coverage, removed test for process creation after VM termination
src/shared/inc/lxinitshared.h Removed WSLA_WAITPID, WSLA_WAITPID_RESULT, and WSLA_SHUTDOWN message structures
src/linux/init/WSLAInit.cpp Removed HandleMessageImpl for WSLA_WAITPID, removed WSLA_WAITPID from HandleMessage template list
msipackage/package.wix.in Removed registry entries for WSLAVirtualMachine and IWSLAVirtualMachine COM classes

expectSessions({});
}

// Validate that persistent sessions are only dropped when explicitely terminated.
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

Spelling error: "explicitely" should be "explicitly".

Suggested change
// Validate that persistent sessions are only dropped when explicitely terminated.
// Validate that persistent sessions are only dropped when explicitly terminated.

Copilot uses AI. Check for mistakes.

PRETTY_PRINT(FIELD(Header), FIELD(Pid), FIELD(Signal));
};

Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

The Linux side still references LxMessageWSLAShutdown in line 832 of WSLAInit.cpp but the WSLA_SHUTDOWN message structure has been removed from lxinitshared.h. While this won't cause immediate issues since the message is never sent, this is now dead code. The check for LxMessageWSLAShutdown should be removed from the Linux-side message processing loop to maintain consistency with the removal of the WSLA_SHUTDOWN message type.

Suggested change
struct WSLA_SHUTDOWN
{
static inline auto Type = LxMessageWSLAShutdown;
using TResponse = RESULT_MESSAGE<int32_t>;
DECLARE_MESSAGE_CTOR(WSLA_SHUTDOWN);
MESSAGE_HEADER Header;
PRETTY_PRINT(FIELD(Header));
};

Copilot uses AI. Check for mistakes.
Comment on lines 1151 to 1154
// Validate that the process object correctly handle requests after the VM has terminated.
VERIFY_SUCCEEDED(session->Shutdown(30 * 1000));
session.reset();
VERIFY_ARE_EQUAL(process.Get().Signal(WSLASignalSIGKILL), HRESULT_FROM_WIN32(ERROR_INVALID_STATE));
}
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

The deleted test case validated that processes cannot be created after the VM is terminated. This is important functionality that ensures the API returns the correct error when attempting to create processes in an invalid state. Removing this test case reduces test coverage for the error handling path in CreateRootNamespaceProcess. Consider keeping this test or adding an equivalent test for the Terminate() functionality.

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +47
// TODO: ACL check.
if (session.DisplayName() == Settings->DisplayName)
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

The TODO comment "ACL check" indicates that security checks are missing. When opening an existing persistent session by name, the code should verify that the caller has permission to access that session. Without ACL checks, any user could potentially open and control sessions created by other users in the same user session context, which could be a security concern depending on the deployment model.

Copilot uses AI. Check for mistakes.
Comment on lines +2423 to +2424
auto session1Copy =
create(L"session-1", static_cast<WSLASessionFlags>(WSLASessionFlagsPersistent | WSLASessionFlagsOpenExisting));
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

The test creates a session with both WSLASessionFlagsPersistent and WSLASessionFlagsOpenExisting flags combined, but there's no explicit test coverage for the error case where WSLASessionFlagsOpenExisting is specified without an existing session. The code should return an appropriate error (likely ERROR_NOT_FOUND) in this case, and this behavior should be tested to ensure the API contract is clear.

Copilot uses AI. Check for mistakes.
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