basepeak-ai/workspace-provider
Watch
6
0
Fork
You've already forked workspace-provider
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(directory): atomic file writes via temp + rename #26

Merged
thklein merged 3 commits from atomic-directory-write into main 2026-06-29 18:18:05 +00:00
Conversation 1 Commits 3 Files changed 2 +122 -4
thklein commented 2026-06-29 18:10:30 +00:00
Owner
Copy link

Problem

The directory workspace provider wrote files via os.OpenFile(..., O_TRUNC) + io.Copy (pkg/client/directory.go). The target was truncated to zero before the new content streamed in, so a process killed mid-write left a truncated/partial file. For knowledge data sources this corrupted .metadata.json, and the corrupt file then aborted the next sync.

Change

directoryProvider.writeFile now writes atomically:

  1. Stream into a temp file in the same directory as the target (same filesystem) via safeopen.OpenFileBeneath (preserving the path-traversal guard) with an O_EXCL + uuid name.
  2. os.Rename over the target — atomic on POSIX within one filesystem, so a reader sees either the complete old file or the complete new file, never a torn one.

Temp files use a reserved .wsp-tmp. prefix that the recursive ls skips, so an in-flight (or crash-leftover) temp never appears in a listing. s3/azure providers already write atomically via a single upload and are untouched.

This is an unconditional fix — no new API or flag. Every directory-provider write (including revision snapshots) becomes atomic transparently.

Notes / tradeoffs

  • Windows: os.Rename over an existing file is not atomic on Windows; deployment is Linux (documented in a code comment).
  • Stale temps on hard crash: a SIGKILL/power-loss between temp-create and rename leaves a .wsp-tmp.<uuid> file. It is hidden from listings by the ls-skip and reclaimed on workspace removal — accepted tradeoff, no reaper added.

Tests

TDD: a new TestWriteFile_AtomicNoTornFileOnError fails against the old O_TRUNC code (a mid-write reader error leaves a torn file) and passes after the change (original content survives). TestLs_SkipsTempFiles covers the listing skip. go build ./..., go vet ./..., and the full pkg/client suite pass.

Related

  • Companion to the bpai-tools .metadata.json hardening (throttled writes + rebuild-on-corrupt) — the tool-side counterpart.
  • Controller-side counterpart: bpai #320 (queue-starvation fix).
## Problem The `directory` workspace provider wrote files via `os.OpenFile(..., O_TRUNC)` + `io.Copy` (`pkg/client/directory.go`). The target was truncated to zero **before** the new content streamed in, so a process killed mid-write left a truncated/partial file. For knowledge data sources this corrupted `.metadata.json`, and the corrupt file then aborted the next sync. ## Change `directoryProvider.writeFile` now writes atomically: 1. Stream into a temp file in the **same directory** as the target (same filesystem) via `safeopen.OpenFileBeneath` (preserving the path-traversal guard) with an `O_EXCL` + uuid name. 2. `os.Rename` over the target — atomic on POSIX within one filesystem, so a reader sees either the complete old file or the complete new file, never a torn one. Temp files use a reserved `.wsp-tmp.` prefix that the recursive `ls` skips, so an in-flight (or crash-leftover) temp never appears in a listing. `s3`/`azure` providers already write atomically via a single upload and are untouched. This is an unconditional fix — no new API or flag. Every directory-provider write (including revision snapshots) becomes atomic transparently. ## Notes / tradeoffs - **Windows:** `os.Rename` over an existing file is not atomic on Windows; deployment is Linux (documented in a code comment). - **Stale temps on hard crash:** a SIGKILL/power-loss between temp-create and rename leaves a `.wsp-tmp.<uuid>` file. It is hidden from listings by the `ls`-skip and reclaimed on workspace removal — accepted tradeoff, no reaper added. ## Tests TDD: a new `TestWriteFile_AtomicNoTornFileOnError` fails against the old `O_TRUNC` code (a mid-write reader error leaves a torn file) and passes after the change (original content survives). `TestLs_SkipsTempFiles` covers the listing skip. `go build ./...`, `go vet ./...`, and the full `pkg/client` suite pass. ## Related - Companion to the bpai-tools `.metadata.json` hardening (throttled writes + rebuild-on-corrupt) — the tool-side counterpart. - Controller-side counterpart: bpai #320 (queue-starvation fix).
fix(directory): write files atomically via temp + rename
All checks were successful
test / test (pull_request) Successful in 1m51s
Details
a98cd8791c
thklein force-pushed atomic-directory-write from a98cd8791c
All checks were successful
test / test (pull_request) Successful in 1m51s
Details
to 35006f4290
All checks were successful
test / test (pull_request) Successful in 1m51s
Details
2026-06-29 18:15:59 +00:00 Compare
thklein commented 2026-06-29 18:16:10 +00:00
Author
Owner
Copy link

Finalize review summary

Fresh-context review (code-reviewer subagent) + local CI parity (gofmt/vet/go test -count=1/build/golangci-lint — all green, 0 issues).

Fixed

  • Test hygiene: TestLs_SkipsTempFiles now cleans up its seeded real.txt and .wsp-tmp.leftover via t.Cleanup, so it can't leak into sibling tests that list the shared workspace root.

Dismissed (with reason)

  • "Double-close tmp on the io.Copy error path" — false positive: the first tmp.Close() is inside the if err != nil branch and is immediately followed by return err; it never coexists with the success-path close. No double-close path exists.
  • "MkdirAll bypasses safeopen's traversal guard" — pre-existing (not touched by this PR) and safe: filepath.Join collapses .. before the join, and the actual file open still goes through safeopen.OpenFileBeneath.

Scope note

  • An unrelated .gitignore chore commit initially rode along; rebased out so this PR is purely the atomic-write change (2 commits).
### Finalize review summary Fresh-context review (code-reviewer subagent) + local CI parity (gofmt/vet/`go test -count=1`/build/golangci-lint — all green, 0 issues). **Fixed** - Test hygiene: `TestLs_SkipsTempFiles` now cleans up its seeded `real.txt` and `.wsp-tmp.leftover` via `t.Cleanup`, so it can't leak into sibling tests that list the shared workspace root. **Dismissed (with reason)** - *"Double-close `tmp` on the `io.Copy` error path"* — false positive: the first `tmp.Close()` is inside the `if err != nil` branch and is immediately followed by `return err`; it never coexists with the success-path close. No double-close path exists. - *"`MkdirAll` bypasses safeopen's traversal guard"* — pre-existing (not touched by this PR) and safe: `filepath.Join` collapses `..` before the join, and the actual file open still goes through `safeopen.OpenFileBeneath`. **Scope note** - An unrelated `.gitignore` chore commit initially rode along; rebased out so this PR is purely the atomic-write change (2 commits).
thklein deleted branch atomic-directory-write 2026-06-29 18:18:05 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something isn't working

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
basepeak-ai/workspace-provider!26
No description provided.