feat(harness): SQLite durability for session state and event replay - #6
Merged
Conversation
Phase 0.5: minimal viable durability. BEAM restart no longer loses session state or event history. New: Harness.Storage GenServer wrapping Exqlite with 2 tables: - harness_events: append-only event log with global_sequence - harness_sessions: materialized session projection Modified: SnapshotServer - Recovers sessions + sequence from SQLite on init - Persists events + session state to SQLite on each apply_event - Falls back to SQLite for replay_since when WAL ring buffer cannot serve the request (e.g., after restart) - Graceful degradation: broadcasts even if SQL write fails Modified: Application supervision tree - Storage starts before SnapshotServer (dependency order) 16 new tests (13 unit + 2 integration + 1 JSON roundtrip): - Event insert/replay/dedup, session upsert/recovery - SnapshotServer recovery: stop → restart → verify state preserved - Replay fallback: restart (WAL empty) → replay_since(0) from SQL Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds a new SQLite-backed persistence GenServer ( Changes
Sequence Diagram(s)sequenceDiagram
participant SnapshotServer
participant StorageGenServer as Storage GenServer
participant SQLiteDB as SQLite DB
participant WALBuffer as WAL Buffer
rect rgba(100,200,150,0.5)
Note over SnapshotServer,SQLiteDB: Startup Recovery
SnapshotServer->>StorageGenServer: recover_from_storage()
StorageGenServer->>SQLiteDB: SELECT sessions
SQLiteDB-->>StorageGenServer: session rows
StorageGenServer->>SQLiteDB: SELECT MAX(global_sequence)
SQLiteDB-->>StorageGenServer: max_seq
StorageGenServer-->>SnapshotServer: sessions + max_seq
SnapshotServer->>WALBuffer: initialize with max_seq
SnapshotServer->>SnapshotServer: rebuild Snapshot state
end
rect rgba(100,150,200,0.5)
Note over SnapshotServer,SQLiteDB: Event Apply & Persistence
SnapshotServer->>SnapshotServer: apply_event()
SnapshotServer->>StorageGenServer: insert_event(event_map)
StorageGenServer->>SQLiteDB: INSERT event row
SQLiteDB-->>StorageGenServer: :ok
SnapshotServer->>StorageGenServer: upsert_session(session_map)
StorageGenServer->>SQLiteDB: INSERT ... ON CONFLICT session row
SQLiteDB-->>StorageGenServer: :ok
StorageGenServer-->>SnapshotServer: :ok
end
rect rgba(200,150,100,0.5)
Note over SnapshotServer,SQLiteDB: Replay on Cache Miss
SnapshotServer->>WALBuffer: replay_since(after_seq)
alt WAL hit
WALBuffer-->>SnapshotServer: events
else WAL miss/empty/evicted
SnapshotServer->>StorageGenServer: replay_since(after_seq)
StorageGenServer->>SQLiteDB: SELECT events WHERE global_sequence > after_seq
SQLiteDB-->>StorageGenServer: event rows
StorageGenServer-->>SnapshotServer: events
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size:XL
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/harness/lib/harness/snapshot_server.ex (2)
98-121:⚠️ Potential issue | 🔴 CriticalFallback replay must not report success unless it returned the full gap.
The SQL path calls
Harness.Storage.replay_since(after_seq)with its default 500-row limit and also turns storage failures into{:ok, seq, []}. Once a client is more than 500 events behind—or SQLite is unavailable—you still reply withcurrentSeq == seqeven though the client is missing events. That also bypasses the full-sync path inapps/harness/lib/harness_web/harness_channel.exthat currently depends on{:gap, ...}.Also applies to: 248-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/snapshot_server.ex` around lines 98 - 121, The fallback SQL path in handle_call ({:replay_since, after_seq}) and the helper replay_from_sql_or_empty must not return {:ok, seq, []} when the storage call is limited (default 500 rows) or fails; instead detect when Harness.Storage.replay_since(after_seq) returns an error or returns fewer events than needed to fill the gap (i.e., rows < seq - after_seq) and reply with a gap tuple (for example {:gap, seq, after_seq}) so the caller will trigger a full sync; update replay_from_sql_or_empty to call Harness.Storage.replay_since(after_seq), check for {:ok, events} length vs expected gap and for {:error, _} and return {:gap, seq, after_seq} in those cases, otherwise return {:ok, seq, events} as before.
127-135:⚠️ Potential issue | 🔴 CriticalA failed SQLite insert can cause sequence reuse after the next restart.
new_seqis assigned and broadcast before the durable insert is known to have succeeded, butinit/1later restoresseqfromHarness.Storage.get_max_sequence/0. If one insert fails and a later insert succeeds, SQLite lags the public sequence, and the next boot can hand out sequence numbers that clients already saw for different events.Also applies to: 170-183, 201-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/snapshot_server.ex` around lines 127 - 135, The code currently increments and broadcasts new_seq in handle_cast ({:apply_event, event}) before the SQLite insert, which can cause sequence reuse because init/1 restores seq from Harness.Storage.get_max_sequence/0; change the flow so the durable persist to SQLite is completed and confirmed before exposing or advancing the in-memory sequence: have persist (or a new function) accept the event and desired seq, perform the insert and return {:ok, seq} or {:error, reason}, only then set new_seq/seq, update snapshot (Projector.project), build event_map (event_to_map), and broadcast; on persist failure do not advance seq or broadcast and handle retry/logging. Ensure related code paths referenced at lines noted (other handle_cast blocks and init/1 using Harness.Storage.get_max_sequence/0) follow the same pattern so durable state is the source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/harness/lib/harness/storage.ex`:
- Around line 14-15: The file currently sets a cwd-relative module attribute
`@default_db_path` = "priv/data/harness.db"; change this to compute the priv path
for the :harness app at runtime so the DB always lives under the app's priv
directory (e.g. use :code.priv_dir(:harness) or Application.app_dir/2 and
Path.join to build "priv/data/harness.db"). Update the single `@default_db_path`
definition and any other places (the occurrences around the block referenced at
117-127) to use the same app-resolved path so the DB location is consistent and
matches the harness/.gitignore pattern.
In `@apps/harness/test/harness/storage_test.exs`:
- Around line 256-257: Replace the timing-based Process.sleep(50) after casting
apply_event/1 by using a deterministic sync barrier: after calling the cast
(apply_event/1) call _ = :sys.get_state(SnapshotServer) to ensure the
SnapshotServer has processed prior messages before continuing; likewise replace
the other sleep at the later location with the same pattern, and where waiting
for process termination was done via Process.alive?/1 switch to using
Process.monitor/1 and assert_receive the :DOWN message to wait for shutdown.
- Around line 37-63: The test setup currently starts replacement processes with
Storage.start_link/1 and SnapshotServer.start_link/1, which makes the test
responsible for cleanup; replace those calls with start_supervised!/1 so ExUnit
takes ownership and automatically cleans them up (e.g., call
start_supervised!({Storage, db_path: ":memory:"}) and similarly for
SnapshotServer) after you stop the application-managed children via
Supervisor.terminate_child(Harness.Supervisor, Storage) and SnapshotServer; keep
the on_exit logic that restarts the application children but remove manual
GenServer.stop of the test-owned pids since start_supervised!/1 handles
shutdown. Also apply the same replacement for the second setup block around
lines 196-229.
---
Outside diff comments:
In `@apps/harness/lib/harness/snapshot_server.ex`:
- Around line 98-121: The fallback SQL path in handle_call ({:replay_since,
after_seq}) and the helper replay_from_sql_or_empty must not return {:ok, seq,
[]} when the storage call is limited (default 500 rows) or fails; instead detect
when Harness.Storage.replay_since(after_seq) returns an error or returns fewer
events than needed to fill the gap (i.e., rows < seq - after_seq) and reply with
a gap tuple (for example {:gap, seq, after_seq}) so the caller will trigger a
full sync; update replay_from_sql_or_empty to call
Harness.Storage.replay_since(after_seq), check for {:ok, events} length vs
expected gap and for {:error, _} and return {:gap, seq, after_seq} in those
cases, otherwise return {:ok, seq, events} as before.
- Around line 127-135: The code currently increments and broadcasts new_seq in
handle_cast ({:apply_event, event}) before the SQLite insert, which can cause
sequence reuse because init/1 restores seq from
Harness.Storage.get_max_sequence/0; change the flow so the durable persist to
SQLite is completed and confirmed before exposing or advancing the in-memory
sequence: have persist (or a new function) accept the event and desired seq,
perform the insert and return {:ok, seq} or {:error, reason}, only then set
new_seq/seq, update snapshot (Projector.project), build event_map
(event_to_map), and broadcast; on persist failure do not advance seq or
broadcast and handle retry/logging. Ensure related code paths referenced at
lines noted (other handle_cast blocks and init/1 using
Harness.Storage.get_max_sequence/0) follow the same pattern so durable state is
the source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b22e2ec-582f-405a-8956-55b53e75e25a
⛔ Files ignored due to path filters (1)
apps/harness/mix.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/harness/.gitignoreapps/harness/lib/harness/application.exapps/harness/lib/harness/snapshot_server.exapps/harness/lib/harness/storage.exapps/harness/mix.exsapps/harness/test/harness/storage_test.exs
Comment on lines
+14
to
+15
| @default_db_path "priv/data/harness.db" | ||
|
|
There was a problem hiding this comment.
Resolve the default DB path via the harness app, not the process working directory.
"priv/data/harness.db" is cwd-relative, so this DB lands in different places depending on where the BEAM is started. In this umbrella layout that also misses the new ignore rule in apps/harness/.gitignore, which only covers apps/harness/priv/data/*.db*.
Suggested fix
- `@default_db_path` "priv/data/harness.db"
+ `@default_db_path` nil
...
defp resolve_db_path(opts) do
cond do
opts[:db_path] ->
opts[:db_path]
config_path = get_in(Application.get_env(:harness, __MODULE__, []), [:db_path]) ->
config_path
true ->
- `@default_db_path`
+ Application.app_dir(:harness, "priv/data/harness.db")
end
endAlso applies to: 117-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/storage.ex` around lines 14 - 15, The file currently
sets a cwd-relative module attribute `@default_db_path` = "priv/data/harness.db";
change this to compute the priv path for the :harness app at runtime so the DB
always lives under the app's priv directory (e.g. use :code.priv_dir(:harness)
or Application.app_dir/2 and Path.join to build "priv/data/harness.db"). Update
the single `@default_db_path` definition and any other places (the occurrences
around the block referenced at 117-127) to use the same app-resolved path so the
DB location is consistent and matches the harness/.gitignore pattern.
Comment on lines
+37
to
+63
| setup do | ||
| # The Application supervisor starts Storage. We need to stop it | ||
| # and restart with :memory: for isolated tests. | ||
| case Process.whereis(Storage) do | ||
| nil -> :ok | ||
| _pid -> Supervisor.terminate_child(Harness.Supervisor, Storage) | ||
| end | ||
|
|
||
| # Also stop SnapshotServer since it depends on Storage | ||
| case Process.whereis(SnapshotServer) do | ||
| nil -> :ok | ||
| _pid -> Supervisor.terminate_child(Harness.Supervisor, SnapshotServer) | ||
| end | ||
|
|
||
| # Start fresh in-memory Storage | ||
| {:ok, _} = Storage.start_link(db_path: ":memory:") | ||
|
|
||
| on_exit(fn -> | ||
| # Restart the Application-managed versions | ||
| case Process.whereis(Storage) do | ||
| nil -> :ok | ||
| pid -> GenServer.stop(pid) | ||
| end | ||
|
|
||
| Supervisor.restart_child(Harness.Supervisor, Storage) | ||
| Supervisor.restart_child(Harness.Supervisor, SnapshotServer) | ||
| end) |
There was a problem hiding this comment.
Start the test-owned processes with start_supervised!/1.
These setups replace app-global Harness.Storage and Harness.SnapshotServer instances with bare start_link/1, which pushes process ownership/cleanup into the test logic. Please switch the replacement processes to start_supervised!/1 once the application-managed children have been stopped.
As per coding guidelines, apps/harness/**/*_test.exs: Always use start_supervised!/1 to start processes in tests as it guarantees cleanup between tests.
Also applies to: 196-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/test/harness/storage_test.exs` around lines 37 - 63, The test
setup currently starts replacement processes with Storage.start_link/1 and
SnapshotServer.start_link/1, which makes the test responsible for cleanup;
replace those calls with start_supervised!/1 so ExUnit takes ownership and
automatically cleans them up (e.g., call start_supervised!({Storage, db_path:
":memory:"}) and similarly for SnapshotServer) after you stop the
application-managed children via Supervisor.terminate_child(Harness.Supervisor,
Storage) and SnapshotServer; keep the on_exit logic that restarts the
application children but remove manual GenServer.stop of the test-owned pids
since start_supervised!/1 handles shutdown. Also apply the same replacement for
the second setup block around lines 196-229.
Comment on lines
+256
to
+257
| # Give cast time to process | ||
| Process.sleep(50) |
There was a problem hiding this comment.
Replace the fixed sleeps with a GenServer sync barrier.
Both waits are timing-based. Since apply_event/1 is a cast, a busy CI runner can still be behind after 50ms. _ = :sys.get_state(SnapshotServer) right after the casts gives you a deterministic “all prior messages handled” barrier before reading the snapshot or restarting the server.
As per coding guidelines, apps/harness/**/*_test.exs: Avoid Process.sleep/1 and Process.alive?/1 in tests; use Process.monitor/1 and assert on the DOWN message to wait for process completion, and In tests, use _ = :sys.get_state/1 to ensure the process has handled prior messages instead of sleeping to synchronize before the next call.
Also applies to: 290-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/test/harness/storage_test.exs` around lines 256 - 257, Replace
the timing-based Process.sleep(50) after casting apply_event/1 by using a
deterministic sync barrier: after calling the cast (apply_event/1) call _ =
:sys.get_state(SnapshotServer) to ensure the SnapshotServer has processed prior
messages before continuing; likewise replace the other sleep at the later
location with the same pattern, and where waiting for process termination was
done via Process.alive?/1 switch to using Process.monitor/1 and assert_receive
the :DOWN message to wait for shutdown.
This was referenced Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harness.Storage— new GenServer wrapping Exqlite with 2 SQLite tables (harness_events+harness_sessions), WAL mode, auto-migrationsSnapshotServerdurability — recovers sessions + sequence from SQLite on init, persists on eachapply_event, falls back to SQL forreplay_sincewhen WAL ring buffer is emptyVerified manually
Session created → harness killed → harness restarted → session visible in snapshot with correct status.
Architecture
Known gap (Phase 1)
After recovery, sessions with status
runningin SQL have no live GenServer. The snapshot shows "running" but the process is dead. Phase 1 adds reconciliation on boot.Test plan
mix compile --warnings-as-errors— cleanmix test test/harness/— 50 tests, 0 failures (16 new + 34 existing)mix credo— clean on new files🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores