fix(memiavl): bound the WAL catch-up wait instead of spinning forever

[JayT106]

What

memiavl/db.go: checkBackgroundSnapshotRewrite's unbounded busy-wait loop is replaced with waitCommittedVersion(targetVersion int64, timeout time.Duration) error, bounded by new constants walCatchupTimeout (5s) and walPollInterval (50ns).

Issue

The loop spun with no timeout waiting for the committed version to catch up, so a stuck writer or unreachable target version hung the caller forever.

Solution

Returns a timeout error instead, propagated through the existing checkAsyncTasks → errors.Join → Commit() chain with no signature changes elsewhere. Confirmed CommittedVersion() doesn't re-lock db.mtx, so no self-deadlock is introduced by polling it.

Test

TestWaitCommittedVersionTimesOut, TestWaitCommittedVersionSucceedsWhenCaughtUp. go test -race ./..., gofmt -l, golangci-lint run all clean.

[JayT106]

@claude

[github-actions]

Claude finished @JayT106's task in 1m 21s —— View job

---

PR Review

• Read changed files and gather context
• Analyze code quality, correctness, and security
• Post inline comments on specific issues
• Write overall review summary

---

Summary

This is a clean, well-scoped fix. The unbounded time.Nanosecond busy-spin is replaced with a proper timeout+poll loop, and the refactoring is minimal — no signature changes, error propagates cleanly through the existing chain. The tests cover both the timeout and the happy-path cases correctly.

Issues Found

One functional bug (inline comment posted): memiavl/db.go line 518 uses == to compare committed version against the target. If the WAL advances past targetVersion between polls (possible under concurrent commits), the condition never triggers and the function will always time out even though the WAL is ahead. Changing to >= is the correct fix.

CodeQL Alerts (non-blocking)

The two time.Now() CodeQL alerts (lines 512 and 521) flag system clock calls as a source of non-determinism. This is expected and intentional here — you need wall-clock time to implement a timeout. These can be dismissed as used in tests / intentional use in production timeout logic.

Minor Observations

• The walCatchupPollInterval of 1ms is a significant increase over the old time.Nanosecond spin, which is strictly better for CPU usage. The tradeoff is up to ~1ms extra latency per snapshot switch, which is negligible.
• The walCatchupTimeout of 5s is reasonable. Consider whether this should be configurable via Options in the future if different deployment environments have different WAL write latency expectations.
• The comment on the constants mentions db.mtx but waitCommittedVersion is actually called while holding db.mtx (inside checkBackgroundSnapshotRewrite → Commit()). The comment is correct in explaining why the timeout is needed; it reads a little confusingly but is not wrong.

Verdict

Approve with one suggestion: fix the == → >= comparison. Everything else looks good.

[JayT106]

@claude

[github-actions]

Claude finished @JayT106's task in 55s —— View job

---

Follow-up Review

• Read current branch state
• Verify previous review issues are resolved
• Post updated review summary

---

All Previous Issues Resolved ✓

The two issues from the first review have both been fixed:

1. == → >= fix (functional bug) — db.go:513 now correctly uses committedVersion >= targetVersion. This prevents a liveness failure when concurrent commits advance the WAL past the target between polls.

2. New test for the >= case — TestWaitCommittedVersionSucceedsWhenWalAdvancesPastTarget (lines 183–217) specifically exercises the scenario where the committed version has already advanced past the target, confirming the >= semantics work correctly. The test comment (line 199) clearly explains why == would have been wrong.

Current State

Check	Status
Unbounded busy-wait replaced with timeout	✓
>= version comparison	✓
Timeout test (TestWaitCommittedVersionTimesOut)	✓
Happy-path test (TestWaitCommittedVersionSucceedsWhenCaughtUp)	✓
Past-target test (TestWaitCommittedVersionSucceedsWhenWalAdvancesPastTarget)	✓
walCatchupTimeout = 5s constant	✓
Poll interval: 50ns (time.Sleep(50 * time.Nanosecond))	✓

Minor Observation

The poll interval is 50 * time.Nanosecond (db.go:519). On most OS schedulers, time.Sleep with a sub-microsecond duration will actually sleep for ~50–100µs (the OS timer resolution floor), so the effective poll rate is already scheduler-bounded rather than literally 50ns. This is fine and avoids the original 100% CPU spin — worth keeping in mind if you ever need to reason about worst-case latency under the 5s timeout.

Verdict

LGTM. The fix is correct, minimal, and well-tested. Ready to merge.