Skip to content

feat(remotefs): add WithContext to bound filesystem operations - #509

Open
kke wants to merge 6 commits into
mainfrom
feat/remotefs-with-context
Open

kke wants to merge 6 commits into
mainfrom
feat/remotefs-with-context

Conversation

@kke

@kke kke commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Adds remotefs.WithContext(ctx, fsys), which returns a copy of a rig filesystem whose commands run under ctx — option 1 from the issue. It also covers the fs.FS-shaped methods and helpers like Upload. Additive: no exported interface changes.

Fixes #474

🤖 Generated with Claude Code

Most filesystem operations ran their commands with a background
context, and the fs.FS-shaped methods have nowhere to take one, so a
host that stopped answering could hang them indefinitely.

WithContext returns a copy of a PosixFS or WinFS whose runner binds
the context-less command methods to ctx. The WinFS directory listing,
the rigrcp file daemon and DownloadURL, which started commands with an
explicit background context, now use the bound one. PosixFS.chtimesFn
holds method expressions so the copy does not run commands through the
original.

Fixes #474

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
@kke
kke requested a balanced review from Copilot October 29, 2026 10:54
@github-actions github-actions Bot added the additive-change Applied by the API diff workflow label Oct 29, 2026
@github-actions

Copy link
Copy Markdown

Exported API changed by this pull request

Compared against aa1a3e7.

This only adds exported API.

Compared for linux, windows, darwin. Entries read ./package.Symbol; the module root package is unprefixed.

Compatible changes (1):

- ./remotefs.WithContext: added

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Context binding can discard third-party wrapper behavior and lose cancellation causes during Windows sessions, while key tests fail before exercising the intended bound-context paths.

4 open issues
What changed in this PR

Adds remotefs.WithContext(ctx, fsys) to let callers bound remote filesystem operations, including standard fs.FS methods and helpers such as Upload, without changing exported interfaces.

Changes:

  • Introduces context-bound copies of POSIX and Windows filesystems.
  • Propagates the bound context to Windows file sessions, directory listings, and URL downloads.
  • Adjusts cached POSIX timestamp handling and adds context-binding tests.
File Description
remotefs/​withcontext.go Adds filesystem copying and context-bound runner methods.
remotefs/​withcontext_test.go Tests binding, cancellation, rebinding, and original filesystem isolation.
remotefs/​winfile.go Uses the bound context for rigrcp sessions.
remotefs/​windir.go Uses the bound context for directory listing commands.
remotefs/​posixfs.go Makes cached timestamp handlers operate on the bound copy.
remotefs/​download.go Passes the filesystem context into URL downloads.

Review effort: Balanced

Comment thread remotefs/winfile.go Outdated
Comment thread remotefs/withcontext.go Outdated
Comment thread remotefs/withcontext_test.go Outdated
Comment thread remotefs/withcontext.go Outdated
WithContext now binds only the exact *PosixFS and *WinFS types, so a type embedding one keeps its own methods, and documents that only remote commands are affected. A rigrcp session ended by the bound context reports that context's error instead of a bare errEnded. The tests now give the Windows listing, file daemon and download paths the responses they need to reach the bound command.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
@kke
kke requested a balanced review from Copilot October 29, 2026 11:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved Windows cancellation bugs allow shell startup to exceed filesystem deadlines, shutdown to report success after cancellation, and failed opens to retain child contexts.

1 open issue
3 previously missed issues

In code that hasn't changed since last review

Medium severity Cancel child context when file daemon startup fails

remotefs/​winfile.go:513

If the initial Stat succeeds but starting the file daemon fails, OpenFile returns no handle and never calls this cancel function. With the new cancellable parent, that leaves the child context registered until the parent's cancellation; repeated failed opens under a long-lived context accumulate retained children. Call cancel() on the Start error path.

Medium severity Preserve cancellation errors during Windows file shutdown

remotefs/​winfile.go:563

Cancellation during Windows file shutdown can still be reported as success: Close ignores the recorded abort cause after its final wait, and closeWithSession discards context errors returned while closing. Consequently, ReadFile and WriteFile can return nil even when cancellation interrupts shutdown. Check the abort cause after Close's final wait and preserve cancellation/deadline errors in closeWithSession (remotefs/winfs.go:116–120), with regression coverage for both paths.

Medium severity Honor operation deadlines during WinRM shell startup

remotefs/​withcontext.go:15

A short filesystem deadline still cannot bound WinRM shell startup: StartProcess synchronously calls client.CreateShell() without the operation context (protocol/winrm/connection.go:449). If the host stops answering there, even the initial Stat can block until the connection's HTTP timeout rather than the filesystem deadline. Make shell creation honor the operation context and test cancellation during startup before promising an end-to-end bound.

4 resolved since last review

Review effort: Balanced

Comment thread remotefs/withcontext.go Outdated
… close

Close and closeWithSession now treat a session that ended with the context as a failure instead of a clean shutdown, and a failed daemon start releases its child context. The WithContext doc now states that a Windows file is closed when its session ends and that WinRM shell creation is bounded only by the connection timeout.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
@kke
kke requested a balanced review from Copilot October 29, 2026 11:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cancellation errors are lost during POSIX stat initialization and Windows session cleanup, preventing callers from recognizing cancellation and allowing Windows reads or writes to report success after cancellation.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Medium severity Propagate cancellation when closing an ended Windows session

remotefs/​winfile.go:747

If cancellation ends the Windows session before deferred Close starts, command("c") returns fs.ErrClosed before this context-error check is reached. closeWithSession ignores that error, so ReadFile and WriteFile can report success despite the canceled session. Return f.abortCause(fs.ErrClosed) from command's closed-file guard, and add a regression test where the session ends before Close starts.

Medium severity Preserve context errors during POSIX stat initialization

remotefs/​withcontext.go:45

On a fresh POSIX filesystem, cancellation before or during the first Stat makes initStat discard the probe errors and report an unsupported stat implementation. Callers then cannot recognize cancellation or deadline expiry with errors.Is, including through OpenFile and uploads. Preserve context errors in PosixFS.initStat before falling back or returning errStatInitFailed, and test cancellation before stat initialization.

1 resolved since last review

Review effort: Balanced

…probe

A Windows file whose session the bound context had already ended returned a bare fs.ErrClosed, which closeWithSession then ignored, and a cancelled first POSIX Stat reported an unsupported stat. Both now carry the context error.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
@kke
kke requested a balanced review from Copilot October 29, 2026 11:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed to evaluate cancellation-induced timestamp precision loss and gaps in Windows in-flight transfer cancellation coverage.

No open issues 🎉

Review effort: Balanced

A nanosecond touch probe cut short by the context fell back to second precision for the lifetime of the filesystem.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
@kke
kke requested a balanced review from Copilot October 29, 2026 11:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The cancellation test retries on the original filesystem instead of the context-bound copy, so it can pass even when cancellation incorrectly caches the seconds-only fallback.

1 open issue

Review effort: Balanced

Comment thread remotefs/withcontext_test.go Outdated
The retry ran on the original filesystem, whose cache the cancelled copy never touched, so caching the fallback there went unnoticed.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
@kke
kke requested a balanced review from Copilot October 29, 2026 11:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed because cancellation can stall Windows file operations until the watchdog fires and cause POSIX probes to cache reduced timestamp precision or misreport cancellation.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

additive-change Applied by the API diff workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

remotefs operations cannot be bounded: no way to pass a context to filesystem commands

2 participants