fix: Tool calling on windows

[Hona]

Replaces global storage lock with per-file lock for write operations and fixes a non awaited operation

fixes #4042

[Copilot]

Pull Request Overview

This PR fixes tool calling issues on Windows by addressing file locking and write operation problems. The changes replace a global storage lock with per-file locks and switch from Bun.write to manual file handle operations with explicit syncing.

Key changes:

• Replaced global storage lock ("storage") with per-file locks using the target file path
• Switched from Bun.write to fs.open with manual writeFile and sync operations for Windows compatibility
• Added directory creation with recursive: true in the write function

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The file handle is opened without checking if the directory exists first. In the update function, unlike the write function (line 203), there's no fs.mkdir(path.dirname(target), { recursive: true }) call before opening the file. This could fail if the parent directories don't exist yet.

Suggested change

	const jsonContent = JSON.stringify(content, null, 2)
	const jsonContent = JSON.stringify(content, null, 2)
	await fs.mkdir(path.dirname(target), { recursive: true })

[Sewer56]

I built this locally, but no ball.

Prompt: Please use ls with bash tool

[Hona]

Weird - it fixed it for me. I'll clean rebuild and try your prompt

[Hona]

This is what I get. Windows 11. What OS/setup do you have?

[Sewer56]

I'm normally on Linux.

But today I had to boot to Windows (once in a blue moon), since I needed to work with some low level WinAPI hooks.
It's 25H2 (Build 26200).

The ls prompt actually worked, but asking it to run multiple tools, it crashed and burned.

[Sewer56]

There's also an oversight in bash tool for win32, it uses realpath binary; which you wouldn't find on Windows.
I've already let @rekram1-node know about that one though; and the canonical Win32 API to resolve, though there's probably a wrapper around it in TS.

[Hona]

Thanks for that extra info @Sewer56

I have got a local repro now, will continue.

[Sewer56]

I don't know the codebase that well (or write TS), but I think I figured it with Sonnet's help.

There's a race condition.

1. Storage.write() called fs.open(target, "w") to write JSON files
2. On Windows, opening a file with mode "w" immediately truncates it to 0 bytes before the write operation completes
3. If another process/thread called Storage.read() during this window, it would read the empty file
4. JSON.parse("") would throw the error

[Hona]

Yeah in theory that is why I made the changes to try and call fsync and so forth for the race conditions. I'll dig into it further

[Sewer56]

diff --git a/packages/opencode/src/storage/storage.ts b/packages/opencode/src/storage/storage.ts
index 804b21ca0..8758ec18f 100644
--- a/packages/opencode/src/storage/storage.ts
+++ b/packages/opencode/src/storage/storage.ts
@@ -157,6 +157,13 @@ export namespace Storage {
     }
   })
 
+  async function atomicRename(source: string, target: string) {
+    if (process.platform === "win32") {
+      await fs.unlink(target).catch(() => {})
+    }
+    await fs.rename(source, target)
+  }
+
   export async function remove(key: string[]) {
     const dir = await state().then((x) => x.dir)
     const target = path.join(dir, ...key) + ".json"
@@ -182,13 +189,15 @@ export namespace Storage {
       const content = await Bun.file(target).json()
       fn(content)
       const jsonContent = JSON.stringify(content, null, 2)
-      const handle = await fs.open(target, "w")
+      const tempTarget = target + ".tmp"
+      const handle = await fs.open(tempTarget, "w")
       try {
         await handle.writeFile(jsonContent, "utf-8")
         await handle.sync()
       } finally {
         await handle.close()
       }
+      await atomicRename(tempTarget, target)
       return content as T
     })
   }
@@ -200,13 +209,15 @@ export namespace Storage {
       using _ = await Lock.write(target)
       const jsonContent = JSON.stringify(content, null, 2)
       await fs.mkdir(path.dirname(target), { recursive: true })
-      const handle = await fs.open(target, "w")
+      const tempTarget = target + ".tmp"
+      const handle = await fs.open(tempTarget, "w")
       try {
         await handle.writeFile(jsonContent, "utf-8")
         await handle.sync()
       } finally {
         await handle.close()
       }
+      await atomicRename(tempTarget, target)
     })
   }
 

This makes it work on my end (what Sonnet spat out), but this can still be improved.

[Hona]

Yup I am trying similar, tmp -> rename. I'll also see if I can revert to bun apis for the file write. Thanks for that

[Sewer56]

Canonical way to do it is MoveFileEx("temp.txt", "original.txt", MOVEFILE_REPLACE_EXISTING); in Win32.
This is atomic (i.e. a file will always be available), as long as source and destination are on the same drive (which of course, they are).

Not sure if TS exposes anything to rename with replacement though.

[Sewer56]

Since the atomicRename output by LLM is not in fact, atomic; given it had to unlink / delete first to work.

[Sewer56]

Are we sure this is the right comment to make?

// Windows: delete then rename (not atomic, but safe with write lock)

The error is more to do with someone else reading it somewhere, right?
So I don't think the write lock would have an effect; as this could be from another thread, etc.

[Hona]

Weirdly online threads seem to think that fs.rename does use MoveFileEx("temp.txt", "original.txt", MOVEFILE_REPLACE_EXISTING); under the hood on windows. But we see errors + the 0 byte issues, so I guess not - maybe not with bun?

[Sewer56]

Maybe it does, but you get a nasty eperm one way or another, unfortunately.
Could it be that there's a handle open to the file that's being moved into? hmm

But then unlink would fail... so probably not.

[Hona]

I'm not a JS guy, but it seems that

// Must await JSON parsing before lock releases
const result = await Bun.file(target).json()
return result as T

is actually the key here. fs.rename now works for me - do you want to test the latest code? @Sewer56

[Sewer56]

@Hona I think I got it cleaner actually, check my branch ; I put it up as draft PR for easier review #4265

[Sewer56]

// Must await JSON parsing before lock releases

You're right on the money, yeah, I noticed the same thing.
And dw, I do 0 things related to webdev too, haha.

I just needed to reboot to Windows once in a blue moon.

[Hona]

Your codes nice too - I'll remove all my extra error handling etc and just keep with bun APIs + that proper await. I'll see how small I can make it

[Sewer56]

Yup, reverted to Bun APIs too.
Seems we just made a small derp earlier in the commits.
We didn't await; so we returned promises while lock was still being held.

All sorted here.

[Hona]

Yep I think we have basically the same outcome now comparing our branches. Works great for me now. Nice to figure it out together :)

[Sewer56]

👏 👏
Rare pair programming moment.

[Copilot]

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Sewer56]

I wonder why the opencode folks were taking lock on storage string though.
Taking a lock on everything seems rather overreaching, especially since (on Linux), there's no forced locks like on Windows.

We changed it so it's a lock on the file path; seems things are still working, so this should in theory be better with less contention.

[Hona]

Yeah it looks like we dont even need temp files. Just the await fixes it. I am curious about the global lock vs target lock

[Sewer56]

WAIT. The bug was here.
Yeah. Opencode was returning an opened file while releasing the lock.

[Sewer56]

We can also make this cleaner with (await Bun.file(target).json()) as T, keeping 1 line.

[Hona]

Yup. We're really just both figuring out the same things at the same times lol.
Re: the file locks instead of global lock I will leave our change in theory but up to @thdxr in review I suppose.

tbh being a C# dev I prefer the 2 lines for the await rather than wrapping in (). I don't mind though

[Sewer56]

If you look at my profile, I've always been writing a lot of .NET 😂
In my case though, it's mostly down to how complex the line is, in this case, pretty simple; so I don't mind the extra brackets.

[Hona]

Oh wow yeah your .NET work looks pretty broad and deep! Super impressed

[Sewer56]

I pinged @rekram1-node in any case (on Discord)

Ah okay, we did some real time pair programming over GitHub Comments (somehow), and isolated it down. Would you be able to get this merged for us? It's a really trivial fix.

Opencode was returning a Promise (which held a file handle open) from the read<T> async function in storage.ts.

Caveat is: There's a lock in place to prevent concurrent access in storage.ts, but this lock wasn't working because we returned a promise. So while we released the lock from the closure, the promise was still active; holding the handle.

Down the road, another file tried to access the same path, and due to a race condition, the file was truncated to 0 bytes as it was opened for writing. (Opening on Windows with write truncates to 0 by default)

It's a pure coincindence this wasn't breaking Linux too; because while the file does not get truncated, it could be being read at the exact time as it was being written, leading to undefined/inconsistent data

[rekram1-node]

What a find u two, damn good work

[Hona]

We're trying to fix the .NET stigmas ;)