refactor(remotefs): track the Windows file position in Go by kke · Pull Request #480 · k0sproject/rig

kke · 2026-09-24T08:12:35Z

winFile.Seek was a round trip to the rigrcp helper, and every later r or w relied on the handle's position staying where the previous command had left it. The position and size now live on winFile, the way PosixFile keeps them, and every r and w command carries the offset it applies to. Seek no longer contacts the host. The end Seek measures from is the size at open or the furthest offset written, whichever is greater, and an empty Write returns without a round trip so it cannot move that end. The handle also remembers whether the caller opened it for reading and for writing, and refuses an operation that access does not allow before contacting the host, as PosixFile does. So an empty write to a read-only file fails too. The access checked is the one the caller asked for, not the wider one some modes need on the host.

This also takes a round trip out of every append. An O_APPEND write used to send "s 0 End" before its "w". Now it sends its w at the size winFile already knows. That size is the real end: the helper opens the file with FileShare.None, so no other writer can grow the file while the handle is open.

The helper used to acknowledge a w before it had even read the payload, and sent nothing once the write was done. A failed write, such as a full disk, surfaced as the reply to whatever command came next, or not at all if none did. With Seek now local, Write would report success and SeekEnd would measure from an end that was never written. The helper now sends a second reply once the payload is in the file, and Write waits for it before moving the position or the end. A write the helper reports as failed ends the session: a failed FileStream.Write can leave part of the payload behind, so the handle no longer knows its position or its end. A read or copy that stops partway ends it too, since the rest of the payload would otherwise be read as the reply to the next command. Each Write now waits for one more reply, which is what it costs to know the data landed.

With that, the helper's "s" command has no caller, so it goes, along with code that never did anything: HexDump, which pasted a dump of the whole short-read buffer into the error message, a FileStream.EndOfStream check on a property FileStream does not have, and assignments nothing read.

Seek stays in the remotefs.File API. This is a step towards replacing the long-lived helper with one-shot commands without losing it.

Refs #475

Copilot

Copilot review overview

🟡 Changes recommended

The size documentation and zero-length write handling must be corrected before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Document size as the furthest position written · New

What changed in this PR

Refactors Windows remote file handling to track offsets locally, eliminating remote seek round trips.

Changes:

Tracks file position and size in Go.
Sends explicit offsets for reads and writes.
Removes obsolete helper seek logic and expands tests.

File	Description	Review
`remotefs/winfs_test.go`	Updates protocol fakes and seek coverage.	No issues.
`remotefs/winfile.go`	Implements local position and size tracking.	Critical (2 votes): Correct the inaccurate size-invariant documentation at lines 227 and 280. Moderate (1 vote): Update `size` only when `written > 0` to prevent zero-length writes beyond EOF from creating unintended holes.
`remotefs/rigrcp.ps1`	Applies explicit offsets and removes seek handling.	No issues.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🟡 Changes recommended

Two moderate write-state and access-mode issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Enforce read-only access checks for zero-byte writes · New

Resolved since last review (1)

Document size as the furthest position written

Copilot

Copilot review overview

🟡 Changes recommended

The critical write-acknowledgement flaw and moderate access-check gap must be resolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Wait for remote write completion before updating position and size · New

Resolved since last review (1)

Enforce read-only access checks for zero-byte writes

Copilot

Copilot review overview

🟡 Changes recommended

Two critical and two moderate correctness issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2

Open (2)

Partial write failure leaves the handle with stale bookkeeping · New
Derived host access incorrectly permits writes on read-only handles · New

Resolved since last review (1)

Wait for remote write completion before updating position and size

Copilot

Copilot review overview

🟡 Changes recommended

Two moderate correctness issues remain in CopyFrom access validation and append-position handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Add access guard to CopyFrom before io.Copy · New

Resolved since last review (2)

Derived host access incorrectly permits writes on read-only handles
Partial write failure leaves the handle with stale bookkeeping

Copilot

Copilot review overview

🔵 Needs a closer look

The read error path can leave the helper session desynchronized and unsafe to reuse.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Add access guard to CopyFrom before io.Copy

…p seek winFile.Seek was a round trip to the rigrcp helper, and every later r or w relied on the handle's position staying where the previous command had left it. The position and size now live on winFile, the way PosixFile keeps them, and every r and w command carries the offset it applies to. Seek no longer contacts the host. The end Seek measures from is the size at open or the furthest offset written, whichever is greater, and an empty Write returns without a round trip so it cannot move that end. The handle also remembers whether the caller opened it for reading and for writing, and refuses an operation that access does not allow before contacting the host, as PosixFile does. So an empty write to a read-only file fails too. The access checked is the one the caller asked for, not the wider one some modes need on the host. The helper used to acknowledge a w before it had even read the payload, and sent nothing once the write was done. A failed write, such as a full disk, surfaced as the reply to whatever command came next, or not at all if none did. With Seek now local, Write would report success and SeekEnd would measure from an end that was never written. The helper now sends a second reply once the payload is in the file, and Write waits for it before moving the position or the end. A write the helper reports as failed ends the session: a failed FileStream.Write can leave part of the payload behind, so the handle no longer knows its position or its end. A read or copy that stops partway ends it too, since the rest of the payload would otherwise be read as the reply to the next command. Each Write now waits for one more reply, which is what it costs to know the data landed. This also takes a round trip out of every append. An O_APPEND write used to send "s 0 End" before its "w". Now it sends its w at the size winFile already knows. That size is the real end: the helper opens the file with FileShare.None, so no other writer can grow the file while the handle is open. With that, the helper's "s" command has no caller, so it goes, along with code that never did anything: HexDump, which pasted a dump of the whole short-read buffer into the error message, a FileStream.EndOfStream check on a property FileStream does not have, and assignments nothing read. Seek stays in the remotefs.File API. This is a step towards replacing the long-lived helper with one-shot commands without losing it. Refs #475 Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Copilot

Copilot review overview

🟡 Changes recommended

The unresolved append correctness and protocol desynchronization issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Prevent stale file size from corrupting concurrent append writes · New

Copilot

Copilot review overview

🔵 Needs a closer look

The stateful cross-language protocol changes warrant final human review despite no identified blocking issues.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Prevent stale file size from corrupting concurrent append writes

Uploads to Windows hosts went through winFile.CopyFrom, which used io.Copy. Upload wraps its source in an io.TeeReader, which hides any io.WriterTo, so io.Copy fell back to its 32 KiB buffer and every 32 KiB became a w command of its own: a reply to wait for before the payload, and since #480, a completion reply after it too. A 100 MiB upload made about 3,200 of those exchanges. CopyFrom now fills 4 MiB at a time, which brings that down to about 25. It does so for every source, including an *os.File, whose own WriteTo would also write 32 KiB at a time. WriteFile had the opposite problem. bytes.Reader implements io.WriterTo, so its whole payload arrived in one Write and went out as a single w. The helper reads a command's whole payload into memory before writing it, and parses its length as a PowerShell [int]. So WriteFile held the entire file in the helper's memory and failed above 2 GiB. Write now splits a larger payload into commands of at most 4 MiB. The idle timeout is unaffected. It is still measured per 32 KiB piece of payload handed to the pipe, not per command, so a larger command does not raise the transfer rate a slow link has to sustain. Refs #475 Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

kke requested a balanced review from Copilot September 24, 2026 08:13

Copilot started reviewing on behalf of kke September 24, 2026 08:13

View session