fix(harness): reconcile stale sessions on recovery - #8
Merged
Conversation
After a BEAM crash/restart, sessions with status running or connecting have no live GenServer backing them. On boot, SnapshotServer now detects these stale sessions and transitions them to error with reason runtime_restarted. Synthetic session/error events are persisted to SQL so the event log stays consistent. Also fix runtime.exs to not override server: false in test env. 6 new tests covering: running→error, connecting→error, ready/closed/error untouched, synthetic event persistence, multi-session reconciliation, active_turn cleared. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConditionally skip starting the HarnessWeb HTTP server in the Changes
Sequence DiagramsequenceDiagram
participant SS as SnapshotServer
participant Storage as Storage Layer
participant SQL as SQL Database
participant Snap as In-memory Snapshot
SS->>Storage: recover_from_storage()
Storage->>SQL: load snapshot, sessions, events
SQL-->>Storage: return persisted data
Storage-->>SS: initial snapshot + seq
SS->>Snap: reconcile_stale_sessions(snapshot, seq)
Note over Snap,SQL: For each session with status :running or :connecting
Snap->>Snap: set status := :error\nactive_turn := nil\nupdated_at := now\nincrement sequence
Snap->>SQL: insert synthetic session/error event
Snap->>SQL: upsert session with error state
SQL-->>Snap: persist result (errors logged)
Snap-->>SS: return reconciled snapshot + seq
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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:M
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: 2
🤖 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/snapshot_server.ex`:
- Around line 217-244: The current reconciliation block in snapshot_server.ex
ignores return values from Harness.Storage.insert_event/1 and
Harness.Storage.upsert_session/1 so {:error, reason} failures are silent; update
the try block around those calls to explicitly handle their results
(pattern-match the return values) — check the result of
Harness.Storage.insert_event(...) and if it returns {:error, reason} log a
descriptive error with inspect(reason) and handle similarly for
Harness.Storage.upsert_session(...); if either call fails, ensure you return or
raise so the reconciliation does not continue silently (or at minimum record the
failure and abort the reconciliation), and keep the existing catch :exit handler
for exit tuples.
In `@apps/harness/test/harness/snapshot_server_test.exs`:
- Around line 98-99: Tests are starting Storage via Storage.start_link which
bypasses ExUnit's supervision; replace each direct call to
Storage.start_link(db_path: ":memory:") with start_supervised!(%{id: Storage,
start: {Storage, :start_link, [[db_path: ":memory:"]]}}) or the shorter
start_supervised!(Storage) variant if Storage.supports child_spec/1 so the
process is started under the test supervisor and will be cleaned up
automatically; update every occurrence that currently calls Storage.start_link
(the instances at the spots referenced in the review) to use start_supervised!/1
instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e81aa823-9413-4775-8ddf-e69b1ef90e01
📒 Files selected for processing (3)
apps/harness/config/runtime.exsapps/harness/lib/harness/snapshot_server.exapps/harness/test/harness/snapshot_server_test.exs
Comment on lines
+98
to
+99
| {:ok, _} = Storage.start_link(db_path: ":memory:") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining direct start_link usages in harness tests
rg -nP --type=elixir '\b(start_link\s*\()' apps/harness/testRepository: Ranvier-Technologies/t3code-OTP
Length of output: 1276
Use start_supervised!/1 instead of direct start_link calls in tests.
Direct start_link calls bypass ExUnit supervision guarantees and make cleanup/state isolation less reliable when tests fail. This violates the guideline that test processes must use start_supervised!/1.
Apply to lines 98, 126, 143, 156, 177, 194, and 212:
Proposed fixes
- {:ok, _} = Storage.start_link(db_path: ":memory:")
+ start_supervised!({Storage, db_path: ":memory:"})- {:ok, _} = SnapshotServer.start_link(nil)
+ start_supervised!(SnapshotServer)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {:ok, _} = Storage.start_link(db_path: ":memory:") | |
| start_supervised!({Storage, db_path: ":memory:"}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/test/harness/snapshot_server_test.exs` around lines 98 - 99,
Tests are starting Storage via Storage.start_link which bypasses ExUnit's
supervision; replace each direct call to Storage.start_link(db_path: ":memory:")
with start_supervised!(%{id: Storage, start: {Storage, :start_link, [[db_path:
":memory:"]]}}) or the shorter start_supervised!(Storage) variant if
Storage.supports child_spec/1 so the process is started under the test
supervisor and will be cleaned up automatically; update every occurrence that
currently calls Storage.start_link (the instances at the spots referenced in the
review) to use start_supervised!/1 instead.
Collaborator
Author
There was a problem hiding this comment.
The setup needs to Supervisor.terminate_child the app-managed Storage first, then start a :memory: instance with different opts. start_supervised! does not handle this terminate-then-restart-with-different-config flow. The on_exit callback restores the app-managed processes, which is why it uses catch_exit (process may already be dead from test cleanup).
There was a problem hiding this comment.
@ranvier2d2, that's a valid point — start_supervised!/1 only manages processes under the ExUnit test supervisor and has no mechanism to first terminate an existing app-supervisor-managed named process before starting a replacement with different options (:memory: vs. the production DB path). The terminate-then-start-with-different-config flow, followed by Supervisor.restart_child in on_exit, requires direct start_link calls here. My suggestion doesn't apply in this context. Acknowledged!
✏️ Learnings added
Learnt from: ranvier2d2
Repo: Ranvier-Technologies/t3code-OTP PR: 8
File: apps/harness/test/harness/snapshot_server_test.exs:98-99
Timestamp: 2026-03-25T18:20:01.513Z
Learning: In `apps/harness/test/harness/snapshot_server_test.exs` (Elixir/ExUnit), the `ReconciliationTest` setup deliberately uses direct `Storage.start_link(db_path: ":memory:")` instead of `start_supervised!/1` because it must first `Supervisor.terminate_child(Harness.Supervisor, Storage)` to stop the app-managed Storage, then start a fresh in-memory instance with different opts. `start_supervised!/1` cannot handle this terminate-then-restart-with-different-config pattern. The `on_exit` callback restores the app-managed processes via `Supervisor.restart_child`, using `catch_exit` because those processes may already be dead. This is a legitimate exception to the `start_supervised!/1` guideline.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: Ranvier-Technologies/t3code-OTP PR: 0
File: apps/harness/AGENTS.md:0-0
Timestamp: 2026-03-24T15:34:43.161Z
Learning: Applies to apps/harness/**/*_test.exs : In tests, use `_ = :sys.get_state/1` to ensure the process has handled prior messages instead of sleeping to synchronize before the next call
Learnt from: CR
Repo: Ranvier-Technologies/t3code-OTP PR: 0
File: apps/harness/AGENTS.md:0-0
Timestamp: 2026-03-24T15:34:43.161Z
Learning: Applies to 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
Learnt from: CR
Repo: Ranvier-Technologies/t3code-OTP PR: 0
File: apps/harness/AGENTS.md:0-0
Timestamp: 2026-03-24T15:34:43.161Z
Learning: Applies to apps/harness/**/*_test.exs : Always use `start_supervised!/1` to start processes in tests as it guarantees cleanup between tests
Learnt from: CR
Repo: Ranvier-Technologies/t3code-OTP PR: 0
File: apps/harness/AGENTS.md:0-0
Timestamp: 2026-03-24T15:34:43.161Z
Learning: Applies to apps/harness/**/*.{ex,exs} : Elixir's builtin OTP primitives like `DynamicSupervisor` and `Registry` require names in the child spec, such as `{DynamicSupervisor, name: MyApp.MyDynamicSup}`, then use `DynamicSupervisor.start_child(MyApp.MyDynamicSup, child_spec)`
Learnt from: CR
Repo: Ranvier-Technologies/t3code-OTP PR: 0
File: apps/harness/AGENTS.md:0-0
Timestamp: 2026-03-24T15:34:43.161Z
Learning: Applies to apps/harness/**/*.{ex,exs} : Don't use `String.to_atom/1` on user input due to memory leak risk
PreToolUse hook blocks gh-pr-create unless --repo Ranvier-Technologies/t3code-OTP is specified. Prevents accidental PRs to the upstream pingdotgg/t3code public repo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/hooks/guard-gh-pr-create.sh:
- Around line 18-21: The guard script's check on COMMAND only matches the
space-separated form of --repo and should accept the equals form too; update the
grep to use an extended regex (e.g., switch to grep -E or grep -P) and match
either "--repo " or "--repo=" before the repository string so the conditional
(the block using COMMAND and the if ... exit 0) allows both "--repo
Ranvier-Technologies/t3code-OTP" and "--repo=Ranvier-Technologies/t3code-OTP".
- Around line 14-16: The current grep pattern in guard-gh-pr-create.sh that
tests COMMAND only checks for '(^|\&\&|\|\||;)' and misses separators like
single pipe '|', background '&', subshell '(', and whitespace/newlines; update
the regex used on COMMAND to include those separators (for example replace the
alternation with a character class such as
'(^|[&|;()[:space:]])\s*gh\s+pr\s+create\b') so the check detects "gh pr create"
preceded by single pipe, background ampersand, subshell start, or any
whitespace/newline; ensure the variable name COMMAND and the grep invocation
(grep -qE ...) are left intact while swapping the pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 335eecc3-deeb-4327-84cc-ccf669d7ded2
📒 Files selected for processing (3)
.claude/hooks/guard-gh-pr-create.sh.claude/settings.json.gitignore
✅ Files skipped from review due to trivial changes (1)
- .claude/settings.json
- Log Storage insert/upsert errors during reconciliation instead of
silently ignoring {:error, reason} returns
- Hook: accept --repo= (equals form) in addition to --repo (space form)
- Hook: skip git commit commands to avoid false positives on messages
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/hooks/guard-gh-pr-create.sh:
- Around line 17-24: The current guard in guard-gh-pr-create.sh incorrectly uses
substring regexes on COMMAND, letting chained commands like "git commit ... &&
gh pr create ..." bypass checks and misdetecting --repo inside other flags; fix
by token-aware parsing: split COMMAND into separate shell commands on separators
(&&, ||, ;, |, newline), then for each command shell-parse respecting quotes
into argv tokens and locate invocations where argv[0] == "gh" and the next
tokens are "pr" and "create"; for that specific argv sequence, inspect its argv
for a --repo argument in either --repo=owner/repo or --repo owner/repo form
(taking quoted tokens into account) and only allow when the repo value exactly
equals "Ranvier-Technologies/t3code-OTP"; remove or tighten the existing echo
grep shortcuts (the git commit shortcut should only allow when the entire
COMMAND is a git commit invocation, not when chained).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ead226e3-c67e-4410-98a1-a717e5b5b16a
📒 Files selected for processing (2)
.claude/hooks/guard-gh-pr-create.shapps/harness/lib/harness/snapshot_server.ex
Comment on lines
+17
to
+24
| # Skip if gh pr create is inside a git commit message (not an actual command) | ||
| if echo "$COMMAND" | grep -qE '^\s*git\s+commit\b'; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Allow if correct --repo is specified (space or equals form) | ||
| if echo "$COMMAND" | grep -qE '\-\-repo[= ]Ranvier-Technologies/t3code-OTP'; then | ||
| exit 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
pattern='--repo[= ]Ranvier-Technologies/t3code-OTP'
cases=(
'git commit -m "msg" && gh pr create'
'gh pr create --title "--repo Ranvier-Technologies/t3code-OTP"'
'gh pr create --repo="Ranvier-Technologies/t3code-OTP"'
)
for c in "${cases[@]}"; do
echo "CASE: $c"
if ! echo "$c" | grep -q 'gh pr create'; then
echo " hook: ALLOW (no gh pr create)"
continue
fi
if echo "$c" | grep -qE '^\s*git\s+commit\b'; then
echo " hook: ALLOW via Line 18"
continue
fi
if echo "$c" | grep -qE "$pattern"; then
echo " hook: ALLOW via Line 23"
else
echo " hook: BLOCK"
fi
doneRepository: Ranvier-Technologies/t3code-OTP
Length of output: 621
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
cat -n ".claude/hooks/guard-gh-pr-create.sh" | head -30Repository: Ranvier-Technologies/t3code-OTP
Length of output: 1289
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "=== Testing Line 18 bypass (git commit prefix) ==="
test_case_1='git commit -m "msg" && gh pr create'
echo "TEST: $test_case_1"
if echo "$test_case_1" | grep -qE '^\s*git\s+commit\b'; then
echo " Result: ALLOW via Line 18 pattern"
else
echo " Result: Does NOT match Line 18 pattern"
fi
echo ""
echo "=== Testing Line 23 false positives (substring matching) ==="
# Test case 2: --repo in title (should NOT allow but current code might)
test_case_2='gh pr create --title "Fix --repo Ranvier-Technologies/t3code-OTP issue"'
echo "TEST: $test_case_2"
if echo "$test_case_2" | grep -qE '\-\-repo[= ]Ranvier-Technologies/t3code-OTP'; then
echo " Result: ALLOW via Line 23 pattern (FALSE POSITIVE if used in title)"
else
echo " Result: Does NOT match Line 23 pattern"
fi
# Test case 3: Valid --repo with equals and quotes (current pattern edge case)
test_case_3='gh pr create --repo="Ranvier-Technologies/t3code-OTP"'
echo "TEST: $test_case_3"
if echo "$test_case_3" | grep -qE '\-\-repo[= ]Ranvier-Technologies/t3code-OTP'; then
echo " Result: ALLOW via Line 23 pattern"
else
echo " Result: Does NOT match Line 23 pattern (valid --repo fails!)"
fi
# Test case 4: Valid --repo with space (should work)
test_case_4='gh pr create --repo Ranvier-Technologies/t3code-OTP'
echo "TEST: $test_case_4"
if echo "$test_case_4" | grep -qE '\-\-repo[= ]Ranvier-Technologies/t3code-OTP'; then
echo " Result: ALLOW via Line 23 pattern (works correctly)"
else
echo " Result: Does NOT match Line 23 pattern"
fi
# Test case 5: Valid --repo with equals (should work)
test_case_5='gh pr create --repo=Ranvier-Technologies/t3code-OTP'
echo "TEST: $test_case_5"
if echo "$test_case_5" | grep -qE '\-\-repo[= ]Ranvier-Technologies/t3code-OTP'; then
echo " Result: ALLOW via Line 23 pattern (works correctly)"
else
echo " Result: Does NOT match Line 23 pattern"
fiRepository: Ranvier-Technologies/t3code-OTP
Length of output: 760
Command-text regex checks are bypassable and can also block valid input.
At Line 18, any command beginning with git commit is auto-allowed, so git commit ... && gh pr create ... bypasses the guard.
At Line 23, substring matching against raw text allows false positives (e.g., repo string inside --title) and blocks valid quoted forms (e.g., --repo="Ranvier-Technologies/t3code-OTP").
Use token-aware parsing of COMMAND and validate --repo as an actual gh pr create argument.
Proposed fix (token-aware check)
-# Skip if gh pr create is inside a git commit message (not an actual command)
-if echo "$COMMAND" | grep -qE '^\s*git\s+commit\b'; then
- exit 0
-fi
-
-# Allow if correct --repo is specified (space or equals form)
-if echo "$COMMAND" | grep -qE '\-\-repo[= ]Ranvier-Technologies/t3code-OTP'; then
- exit 0
-fi
+# Parse argv safely and validate --repo as a real gh argument token.
+PARSE_RESULT="$(
+python3 - "$COMMAND" <<'PY'
+import shlex, sys
+
+TARGET = "Ranvier-Technologies/t3code-OTP"
+cmd = sys.argv[1]
+
+try:
+ argv = shlex.split(cmd)
+except ValueError:
+ print("malformed")
+ sys.exit(0)
+
+if len(argv) >= 3 and argv[:3] == ["gh", "pr", "create"]:
+ repo = None
+ i = 3
+ while i < len(argv):
+ a = argv[i]
+ if a == "--repo" and i + 1 < len(argv):
+ repo = argv[i + 1]
+ i += 2
+ continue
+ if a.startswith("--repo="):
+ repo = a.split("=", 1)[1]
+ i += 1
+ print("allow" if repo == TARGET else "block")
+else:
+ print("skip")
+PY
+)"
+
+case "$PARSE_RESULT" in
+ skip|allow) exit 0 ;;
+esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/guard-gh-pr-create.sh around lines 17 - 24, The current guard
in guard-gh-pr-create.sh incorrectly uses substring regexes on COMMAND, letting
chained commands like "git commit ... && gh pr create ..." bypass checks and
misdetecting --repo inside other flags; fix by token-aware parsing: split
COMMAND into separate shell commands on separators (&&, ||, ;, |, newline), then
for each command shell-parse respecting quotes into argv tokens and locate
invocations where argv[0] == "gh" and the next tokens are "pr" and "create"; for
that specific argv sequence, inspect its argv for a --repo argument in either
--repo=owner/repo or --repo owner/repo form (taking quoted tokens into account)
and only allow when the repo value exactly equals
"Ranvier-Technologies/t3code-OTP"; remove or tighten the existing echo grep
shortcuts (the git commit shortcut should only allow when the entire COMMAND is
a git commit invocation, not when chained).
5 tasks
ranvier2d2
added a commit
that referenced
this pull request
Mar 30, 2026
…handler - Guard against double subscription: lease_and_subscribe and subscribe_initial check if thread_id is already in subscribers before creating a new monitor, preventing ref count leak from duplicate monitors (#5). - Handle :runtime_sse_degraded in session: emit session/degraded event when runtime SSE reconnect is exhausted, instead of silently dropping the message (#8). - Add Logger.debug to event_relevant? catch-all so unrecognized SSE event shapes are logged instead of silently dropped (#10). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
runningorconnectingnow transition toerrorwith reasonruntime_restartedsession/errorevents persisted to SQL event log for consistencyruntime.exsoverridingserver: falsein test env (was causing port 4321 conflicts)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores