fix: prevent data loss during S3 outages with WAL-based recovery

[khalid244]

Summary

Fixes critical data loss bugs when S3 becomes unavailable. Data is preserved in WAL and automatically recovered when S3 is healthy again.

Key Changes

WAL Recovery - Data stays in WAL if flush fails, recovered on restart or via periodic recovery
No Memory Growth - Buffer cleared on flush failure (data safe in WAL)
Periodic Recovery - Background goroutine replays WAL every 5 minutes
WAL File Deletion - Files deleted after successful recovery (not renamed to .recovered)

Flow During S3 Outage

Data In -> WAL (disk) -> Buffer (memory) -> Flush -> S3 FAIL
                                                        |
Buffer cleared (memory freed) <-------------------------+
Data preserved in WAL
Periodic recovery replays when S3 recovers
WAL file deleted after successful recovery

---

Changes by File

cmd/arc/main.go

Scenario	Before	After
WAL Recovery Timing	Runs before ArrowBuffer exists	Runs after ArrowBuffer is ready
Recovery Callback	Just logs, doesn't replay data	Actually replays to ArrowBuffer
S3 Outage Recovery	Requires restart	Periodic recovery every 5 minutes

internal/ingest/arrow_writer.go

Scenario	Before	After
Flush Queue Full	dropping task - unclear fate	preserved in WAL - data safe
Async Flush Failure	Silent error, no record count	Logs record count, WAL safety note
Sync Flush Failure	No logging at all	Warns with record count
Memory During Outage	Could grow indefinitely	Bounded - data stays in WAL only

internal/wal/recovery.go

Scenario	Before	After
Entry Failure	continue - processes remaining	break - stops, retries later
After Recovery	Rename to .recovered	Delete file
Partial Failure	File renamed anyway (data lost)	File kept for retry
Stats Tracking	Always counts as recovered	Only counts if ALL succeed
Logging	No entry count	Shows entry count

---

End-to-End Scenarios

Scenario	Before	After
S3 goes down during flush	Data lost or memory grows	Data safe in WAL, memory bounded
S3 recovers	Must restart to recover	Auto-recovers within 5 minutes
Recovery succeeds	.recovered files pile up	Files deleted, disk freed
Recovery partially fails	Lost entries, file archived	File kept, retry on next cycle
Startup recovery	Logs only, data lost	Actually replays all data

---

Test Plan

• Deploy to Kubernetes (arc namespace)
• Ingest data during S3 outage (503 errors)
• Verify data preserved in WAL log message
• Restart pod, verify WAL recovery runs
• Verify WAL file recovered and deleted log message
• Confirm WAL directory only has new empty file

[xe-nvdk]

Review: PR #162 - WAL-based S3 Recovery

Thanks for identifying this issue - you're correct that our WAL recovery callback was essentially a no-op since ArrowBuffer isn't available at that point. This is a legitimate bug that needs fixing.

However, I have some concerns about the implementation that need to be addressed before we can merge.

---

1. Code Duplication (~50 lines repeated)

The recovery callback logic is copy-pasted twice in main.go:

• Lines 309-356 (startup recovery)
• Lines 371-402 (periodic recovery)

Request: Extract this into a shared function, something like:

func createWALRecoveryCallback(arrowBuffer *ingest.ArrowBuffer, logger zerolog.Logger) wal.RecoveryCallback {
    return func(ctx context.Context, records []map[string]interface{}) error {
        // ... shared logic
    }
}

---

2. Hardcoded 5-Minute Interval

The periodic recovery interval is hardcoded:

ticker := time.NewTicker(5 * time.Minute)

Request: Make this configurable via cfg.WAL.RecoveryInterval with a sensible default (5 minutes is fine). Add it to the config struct and documentation.

---

3. Missing Tests

This PR adds critical durability behavior but no tests. Please add:

• Unit test for the recovery callback (mock WriteColumnarDirect)
• Integration test simulating S3 failure → WAL accumulation → recovery
• Test for partial recovery failure (ensure file is kept for retry)

---

4. Recovery During Active WAL Write

When periodic recovery runs, what happens if the WAL writer is actively appending to the current file? The findWALFiles() will return the active file, and we might try to recover entries that haven't been synced yet.

Request: Either:

• Skip the current/active WAL file during recovery (check by filename timestamp vs current WAL)
• Or rotate the WAL before recovery to ensure clean boundaries

---

5. No Backpressure During Mass Recovery

If S3 is down for hours, WAL files accumulate. When S3 recovers, periodic recovery tries to replay everything at once. This could:

• Overwhelm the ArrowBuffer
• Cause memory pressure
• Create a thundering herd of flushes

Request: Add rate limiting or batching to recovery replay.

---

Minor Comments

arrow_writer.go - The new log messages are helpful. Consider adding a metric counter for "records preserved in WAL for recovery" so operators can track this.

recovery.go - The change from continue to break on entry failure is correct. Good catch on the partial failure handling.

---

Summary

The core idea is sound and addresses a real bug. Please address the above concerns, especially:

1. Extract shared recovery callback
2. Make interval configurable
3. Add tests
4. Handle active WAL file during periodic recovery

Happy to discuss any of these points!

[khalid244]

Addressed All Review Feedback

Thanks for the detailed review! I've implemented all the requested changes:

1. ✅ Extracted Shared Recovery Callback

Created createWALRecoveryCallback() function in main.go:981 that's used by both startup and periodic recovery, eliminating ~50 lines of duplication.

2. ✅ Configurable Recovery Interval

Added two new config options to WALConfig:

• RecoveryIntervalSeconds (default: 300 = 5 minutes)
• RecoveryBatchSize (default: 10000 records)

Can be configured via:

[wal]
recovery_interval_seconds = 300
recovery_batch_size = 10000

Or environment variables:

ARC_WAL_RECOVERY_INTERVAL_SECONDS=600
ARC_WAL_RECOVERY_BATCH_SIZE=5000

3. ✅ Added Tests

New tests in wal_test.go:

• TestRecovery_SkipActiveFile - Verifies active file is skipped
• TestRecovery_BatchSize - Verifies rate limiting splits large batches
• TestRecovery_PartialFailure_KeepsFileForRetry - Verifies file retention on failure
• TestRecovery_BatchSize_PartialBatchFailure - Verifies partial batch failure handling

New tests in config_test.go:

• TestWALConfig_Defaults - Verifies default values
• TestWALConfig_EnvOverride - Verifies env var overrides

4. ✅ Skip Active WAL File During Periodic Recovery

Added RecoveryOptions struct with SkipActiveFile field. Periodic recovery now calls walWriter.CurrentFile() and passes it to skip the file being actively written.

5. ✅ Backpressure During Mass Recovery

Added BatchSize option to RecoveryOptions. Large WAL entries are split into smaller batches (default 10,000 records) to prevent overwhelming ArrowBuffer during mass recovery after prolonged outages.

6. ✅ Added WAL Metrics

New Prometheus metrics in metrics.go:

• arc_wal_records_preserved_total - Records preserved in WAL for recovery (flush failures)
• arc_wal_recovery_total - Successful WAL recovery operations
• arc_wal_recovery_records_total - Total records recovered from WAL

---

Test Results:

• All WAL tests pass (21 tests, 83.1% coverage)
• All config tests pass (34 tests, 94.2% coverage)
• go vet clean

[xe-nvdk]

Code looks good. All feedback addressed:

• Extracted shared callback - createWALRecoveryCallback() eliminates duplication
• Configurable recovery interval - recovery_interval_seconds (default 300s)
• Configurable batch size - recovery_batch_size (default 10000) for backpressure
• Skip active WAL file - Periodic recovery skips file being written
• WAL metrics - wal_records_preserved_total, wal_recovery_total, wal_recovery_records_total
• Comprehensive tests - 4 new test cases

PR has merge conflicts with main. Please rebase and resolve, then we can merge.