Skip to content

internal/store tests fail on Windows: t.TempDir cleanup races SQLite handles #728

Description

@Rene-Kuhm

📋 Pre-flight Checks

  • I have searched existing issues and this is not a duplicate
  • I understand this issue needs status:approved before a PR can be opened

🐛 Bug Description

go test ./internal/store/... fails on Windows on current main. Two tests are
red on every run:

  • TestMigrate_Idempotent
  • TestNewErrorBranches

The cause is not the code under test — it is how the temp directory is cleaned
up. These tests use t.TempDir(), which removes the directory as soon as the
test returns. On Windows a file cannot be unlinked while a handle to it is still
open, and SQLite still holds engram.db at that moment, so RemoveAll fails.
Since Go treats a failed t.TempDir() cleanup as a test failure, the test goes
red even though the assertions passed.

On Linux and macOS the same code is fine, because unlinking an open file is
allowed there. That is probably why CI has not caught it.

This came up while I was looking at #550 — that PR touched the same area, so I
wanted to write the underlying problem up separately rather than leave it
implicit.

🔁 Steps to Reproduce

On Windows:

go test ./internal/store/... -count=1

✅ Expected Behaviour

internal/store passes on Windows as it does on Linux and macOS.

❌ Actual Behaviour

--- FAIL: TestMigrate_Idempotent (1.89s)
    testing.go:1464: TempDir RemoveAll cleanup: unlinkat
    C:\...\Temp\TestMigrate_Idempotent3559259003\001\engram.db:
    The process cannot access the file because it is being used by another process
--- FAIL: TestNewErrorBranches (1.92s)
FAIL    github.com/Gentleman-Programming/engram/internal/store

Reproduced on consecutive runs, so it is deterministic rather than flaky.

Environment: Windows 11, go test run locally against main.

📦 Affected Area

internal/store — test helpers only. No production code is involved.

📎 Additional Context

I have a fix ready if it is useful. It is test-only and about 70 lines:

  • A closeAndRemoveStore(t, dir, s) helper that closes the store first, then
    retries RemoveAll for up to 5s to cover the window before the OS releases
    the handle.
  • The affected tests switch from t.TempDir() to os.MkdirTemp plus that
    helper.
  • If the retries are exhausted it logs rather than fails, so a leftover temp
    directory never turns into a spurious red build.

With it, go test ./internal/store/... passes on Windows, and behaviour on
Linux and macOS is unchanged since RemoveAll succeeds on the first attempt
there.

I may well be missing a simpler angle — if you would rather solve it a
different way, or skip it because Windows is not a supported target, that is
completely fine and I would rather know than guess. Thanks either way, and
thanks for engram.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions