fix: Resolve TOCTOU race condition in data version allocation#359
fix: Resolve TOCTOU race condition in data version allocation#359
Conversation
Use Deno.mkdir() (non-recursive) as an atomic claim mechanism for version directory creation. On AlreadyExists collision, increment and retry. This eliminates the read-then-create race between concurrent allocateVersion() and save() calls. Closes #327 Co-Authored-By: Magistr <2269529+umag@users.noreply.github.com>
There was a problem hiding this comment.
✅ LGTM - Approved
This PR correctly fixes the TOCTOU race condition in version allocation. The implementation is well-designed and follows best practices.
What's Good
Correct Fix: Using Deno.mkdir() without recursive: true as an atomic claim mechanism is the right approach. The kernel-level atomicity of mkdir provides a reliable lock without introducing additional infrastructure.
Code Quality:
- TypeScript strict mode compliance - no
anytypes - Proper named exports
- License header present
- Clean error handling with proper re-throw of non-collision errors
Test Coverage:
- Two new tests directly verify the race condition fix
- Tests use proper isolation with
Deno.makeTempDir() - Cleanup in
finallyblocks ensures no test pollution
DDD Alignment:
atomicAllocateVersionDiris correctly a private implementation detail- Repository pattern maintained - persistence concerns stay in infrastructure layer
- No domain logic changes required
Minor Suggestions (Non-blocking)
-
Unused return value:
atomicAllocateVersionDirreturns{ version, versionDir }but callers only useversion. Consider returning just{ version }for clarity, though this is harmless as-is. -
Content verification: The concurrent save test verifies content is not null, but could verify each version contains the expected
content-Nstring to confirm no overwrites occurred. Current test is sufficient to catch the bug though.
Summary
The fix is correct, well-tested, and follows the project's code style guidelines. The approach is elegant - leveraging OS-level atomicity rather than introducing application-level locking.
Summary
Fixes #327 — TOCTOU race condition in
unified_data_repository.tswhere concurrentallocateVersion()andsave()calls could compute the same version number, causing silent data loss.Reasoning
Both
allocateVersion()andsave()followed a read-then-create pattern: read existing version directories, computemax + 1, then create the directory withensureDir(). BecauseensureDir()usesrecursive: true, it silently succeeds even if the directory already exists — so two concurrent calls that both compute version N would both "succeed", with the last writer silently overwriting the first.This is a real risk because workflow steps execute in parallel via
Promise.allSettled()and jobs execute in parallel viaPromise.all().The fix uses
Deno.mkdir()withoutrecursive: trueas an atomic claim mechanism.mkdiris an atomic kernel-level operation — it either creates the directory or throwsAlreadyExists. On collision, we increment the version and retry. This avoids introducing any locking infrastructure; the directory creation itself is the lock.Plan vs Implementation
allocateVersion()(10 parallel calls, assert unique versions) and concurrentsave()(10 parallel calls with distinct content, assert unique versions + content preserved). UsesDeno.makeTempDir(),Data.create(),Promise.all().atomicAllocateVersionDir()private methodgetDataNameDir(). Ensures parent dir withrecursive: true, reads starting version vialistVersions(), loops with non-recursiveDeno.mkdir(), catchesAlreadyExiststo retry with incremented version, rethrows other errors, caps at 100 retries.allocateVersion()listVersions+ensureDirblock withatomicAllocateVersionDir()call. Ownership validation unchanged.save()ensureDirimportimport { ensureDir } from "@std/fs"removed — no other usages in the file.No deviations from the plan. All steps implemented exactly as specified.
Files Changed
src/infrastructure/persistence/unified_data_repository.ts— core fix (atomic version allocation)src/infrastructure/persistence/unified_data_repository_test.ts— concurrent race condition testsTest plan
deno check— type checking passesdeno lint— linting passesdeno fmt— formatting passesdeno run test— all 1713 tests passdeno run compile— binary compiles successfully🤖 Generated with Claude Code