Skip to content

feat: implement & test open_at#363

Merged
Sl1mb0 merged 25 commits intomainfrom
tm/open-at
Mar 10, 2026
Merged

feat: implement & test open_at#363
Sl1mb0 merged 25 commits intomainfrom
tm/open-at

Conversation

@Sl1mb0
Copy link
Copy Markdown
Contributor

@Sl1mb0 Sl1mb0 commented Feb 9, 2026

Closes #336

I followed the specification described here. Given that we will eventually need to test this against the wasi test-suite, any differences between the aforementioned spec and the test-suite will favor the test-suite; so significant changes may need to be made in the future.

Describe your proposed changes here.

  • I've read the contributing section of the project CONTRIBUTING.md.
  • Signed CLA (if not already signed).

@Sl1mb0 Sl1mb0 force-pushed the tm/open-at branch 12 times, most recently from a5a3acf to 967f281 Compare February 10, 2026 02:07
Copy link
Copy Markdown
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

Implements POSIX/WASI-like open_at behavior in the in-memory VFS so guests can open existing nodes and create/truncate files (a prerequisite for enabling host-side FS writes), and updates unit/integration tests to reflect the new semantics.

Changes:

  • Refactors VFS path resolution and expands VfsCtxView::open_at to handle CREATE, EXCLUSIVE, DIRECTORY, and TRUNCATE.
  • Adds extensive unit tests around open_at behavior and updates integration test expectations/snapshots.
  • Makes Limiter::grow take &self (internally synchronized), simplifying ownership/mutability in component setup.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
host/src/vfs/mod.rs Implements new open_at semantics, factors path traversal helper, and adds unit tests.
host/src/limiter.rs Changes grow to &self to allow shared use behind a mutex.
host/src/component.rs Adjusts limiter initialization to match the new Limiter::grow signature/usage.
host/tests/integration_tests/python/runtime/fs.rs Updates Python FS integration test to exercise create-on-open behavior.
host/tests/integration_tests/evil/fs.rs Updates large snapshot expectations for new VFS/open behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Sl1mb0 Sl1mb0 force-pushed the tm/open-at branch 9 times, most recently from d7bd5c4 to adb81f9 Compare February 10, 2026 04:57
@Sl1mb0 Sl1mb0 force-pushed the tm/open-at branch 7 times, most recently from 71e926e to c750feb Compare February 19, 2026 15:42
@Sl1mb0 Sl1mb0 requested a review from crepererum February 20, 2026 16:13
@Sl1mb0 Sl1mb0 force-pushed the tm/open-at branch 2 times, most recently from 6fdcfb6 to efcb604 Compare March 5, 2026 09:02
Comment on lines +1032 to +1034
// Root directory is always at index 0, so we can check if it already
// exists before pushing a new descriptor
let res: Resource<VfsDescriptor> = Resource::new_own(1);
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.

You cannot assume that. The resource table is used by all kinds of WASI things, e.g. the HTTP implementation too. Now if someone would call for example an HTTP method before using the VFS, this assumption will break. In general, hard-coding IDs like this isn't super robust. I would rather that you create add some Option<Resource<Descriptor>> to VfsState and fill it with the resource resource the first time this method is called.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oooof - yeah I do not intend to keep this, this was me playing around with get_directories to see how if it did anything for the BadDescriptor error, but we now know that's not the case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

also re: creating a Option<Resource<Descriptor>> in VfsState - if we need to do that, I'd like to do so in a follow up; I really want to get this merged!

Copy link
Copy Markdown
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

Thank you for pushing this all the way over the finish line 🙏

@Sl1mb0 Sl1mb0 added this pull request to the merge queue Mar 10, 2026
Merged via the queue into main with commit a569182 Mar 10, 2026
2 checks passed
@Sl1mb0 Sl1mb0 deleted the tm/open-at branch March 10, 2026 16:31
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.

Refactor HostDescriptor::open_at for VfsCtxView to allow writes

4 participants