You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(remotefs): add WithContext to bound filesystem operations - #509
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.
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.
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.
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.
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.
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.
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.
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
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.
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.
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.
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.
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>
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>
… 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>
…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>
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>
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>
A command interrupted by its context can report only how it ended. An
ssh session that exits within the grace period after the interrupt
returns its exit status or signal, with nothing saying the context
ended, so callers could not tell a cancellation from a failure.
When the context is done, wrap its error into a failed Wait or start,
keeping the command's own error alongside it. A command that finished
regardless still succeeds.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The cancelled-probe test made the touch command return context.Canceled
itself. An interrupted command reports how it exited instead, so the
probe saw a plain error and cached the seconds-only fallback anyway. It
now returns an exit error and relies on the executor to add the context
error.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
An operation on a winFile waited on the session's done channel, which
closes only once the transport returns from Wait. With the bound context
done, a transport slow to give up held the operation until the
watchdog fired.
Abort the session through context.AfterFunc when the bound context ends,
as the watchdog does: closing the pipes releases whatever is parked on
them with the context's error. Close also stops waiting for the helper
to exit once the context is done.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Cancellation during the actual stat command can still be misreported as a missing file, and the cancellation test lacks synchronization to verify that a parked command is released.
A stat interrupted by its context can exit with a positive status, such
as 130 after SIGINT, which commandRanAndFailed took for stat saying the
path is not there. The executor now adds the context error to such a
failure, so treat that as a cancellation rather than an answer.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Cancelling straight after starting the command could close the file
before the command ran, so the test passed through the closed-file
check without covering the release of a parked command.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
WriteFile closed through closeWithSession, which passes on only a close
that a timeout or the context ended, so a host that refused the close
left WriteFile reporting success. The helper finishes the write when it
closes the file, so for a write any close failure means the data may
not be there. Reads keep closeWithSession: the data is already read.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Every command fails once the context a filesystem is bound to ends, the
cleanup included, so Upload ran a RemoveAll that could only fail and
dropped the error. Skip it as for a session timeout, and document that a
cancelled Upload or DownloadURL can leave its temporary file behind.
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because SSH cancellation can trigger synchronous trace cleanup that blocks indefinitely, preventing execution and bound filesystem operations from completing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
remotefs.WithContext(ctx, fsys), which returns a copy of a rig filesystem whose commands run underctx— option 1 from the issue. It also covers thefs.FS-shaped methods and helpers likeUpload. Additive: no exported interface changes.Fixes #474
🤖 Generated with Claude Code