k0sproject / k0sproject/rig

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

Open
#474 0 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Go
Stars
53
Forks
33
Avg merge
1d 9h
Merged PRs (30d)
29

Description

## What

`remotefs` filesystem operations run their commands with `context.Background()`, so there is no way for a caller to bound them. On a host that has stopped answering, a single `Stat` or `Sha256` blocks for as long as the connection takes to notice — which, for WinRM with no deadline, is forever.

61 call sites across `remotefs/posixfs.go` (35), `remotefs/winfs.go` (21) and `remotefs/posixfile.go` (5) run a command with no caller-supplied context.

## Why now

Came out of #473, which bounded the rigrcp session behind a Windows file so that a session dying *mid-transfer* fails instead of hanging. Copilot rightly pointed out during review that this does not make the public call bounded end to end:

- `WinFS.OpenFile` starts with a `Stat` probe, which runs `ExecOutput` on a background context. An `Upload` against a host that went away before the transfer starts still hangs there, before the new watchdog is ever armed.
- `Upload`'s cleanup calls `RemoveAll`, which starts with a `Stat` on the same connection. #473 works around that specific one by skipping cleanup after a session timeout, but that is a patch on one path, not a fix.

The `protocol/winrm` contract after #473 is explicit: a background context stays unbounded, and a caller wanting a bound sets a deadline. `remotefs` has no way to express that.

## The constraint

This is an API shape problem, not an oversight. `remotefs.FS` embeds `fs.FS`, `fs.StatFS`, `fs.ReadFileFS` and `fs.ReadDirFS`, whose signatures are fixed by the standard library — `Open(name string)`, `Stat(name string)` and friends have nowhere to put a `context.Context`, ever.

Rig's own methods are not stuck that way, and the interface is already inconsistent about it: `remotefs.OS` has `Follow(ctx, path, w)` and `Reboot(ctx)` taking contexts, while `Remove`, `Mkdir`, `WriteFile`, `Rename`, `Sha256` and the rest do not. `cmd.Runner` already embeds `ContextRunner`, so the plumbing exists below this layer.

## Possible directions

Not asking for a specific design, but the options seem to be:

1. **A context-carrying derived FS** — something like `fsys.WithContext(ctx) FS`, returning an FS whose commands all run under `ctx`. Additive, and the only option that also covers the `fs.FS`-shaped methods.
2. **Context variants on rig's own methods** — `RemoveContext`, `Sha256Context` and so on. Additive but partial: it cannot reach `Open`/`Stat`/`ReadFile`/`ReadDir`, which are where the `Upload` hangs above actually happen.
3. **A default command timeout on the connection or runner**, so that every command is bounded even when nobody passes a context. Cheapest, but it changes behaviour for long-running commands, which is exactly why #473 deliberately left background contexts unbounded.

(1) looks like the one worth exploring first. It is worth deciding before more callers grow their own per-path workarounds like the one #473 added to `Upload`.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by tracing the 61 command call sites in remotefs/posixfs.go, remotefs/winfs.go, and remotefs/posixfile.go, then inspect remotefs.FS, remotefs.OS, and cmd.Runner's ContextRunner plumbing. Compare the existing context handling from #473 with the proposed derived-FS direction; done means a decided API can bound both rig-specific operations and fs.FS-shaped methods without leaving background-context calls.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.