Repository navigation
docs(adr): durable session retention + graceful drain (0070) + ArgoCD GitOps (0071) - #608
Merged
Merged
Conversation
ADR-0070: wire the existing durable layer (ADR-0055/0067/0068) into a restart-recovery closed loop so single-replica live HTTP sessions survive a process restart — create-time persistence (P1), graceful drain (P2), boot rehydration + interrupted-turn resume (P3). Multi-replica HA explicitly out of scope. Motivated by live session loss on REVISION 6 rollout + the #606 config-checksum auto-roll putting restart on the hot path. ADR-0071: adopt ArgoCD pull-based GitOps as the target deploy mechanism (private values repo, immutable :sha promotion via Image Updater, manual sync with diff preview), replacing manual SSH helm upgrade. Includes the deployment contract. P2 graceful drain is the prerequisite. Both Proposed; implementation pending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughTwo Architecture Decision Records are updated and added to ChangesADR-0070: Restart-Safe Live Sessions
ADR-0071: ArgoCD GitOps Deployment
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coding-agent/docs/adr/0071-argocd-gitops-deployment.md (1)
1-105:⚠️ Potential issue | 🟠 MajorCorrect release/namespace name in acceptance criteria and fix incomplete criterion formatting.
The ADR structure and decision rationale are sound, but two acceptance-criteria issues need correction:
Line 84: Incorrect release name. The criterion hardcodes
coding-agent-deepseek, but the deploy script defaults tocoding-agent(bothHELM_RELEASEandK8S_NAMESPACEdefault tocoding-agent, notcoding-agent-deepseek). Update line 84 to usecoding-agent, or clarify ifcoding-agent-deepseekis an o6n-prod-specific override with different environment-specific naming.Line 92–93: Incomplete acceptance criterion. The final criterion is a bare test command. Rephrase it as a criterion description, e.g., "Existing deploy-script and helm-chart tests pass:
uv run pytest tests/deploy/test_deploy_script.py tests/deploy/test_helm_chart.py -q".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/docs/adr/0071-argocd-gitops-deployment.md` around lines 1 - 105, In the Acceptance Criteria section, two corrections are needed: First, on line 84, replace the hardcoded release name `coding-agent-deepseek` with `coding-agent` to match the defaults in the deploy script (where both HELM_RELEASE and K8S_NAMESPACE default to `coding-agent`), unless this is an environment-specific override that should be clarified in the criterion text. Second, on lines 92-93, the final acceptance criterion is incomplete and consists only of a bare test command; rephrase it as a proper criterion description (e.g., starting with an action verb and outcome) that includes the test command, such as "Existing deploy-script and helm-chart tests pass: `uv run pytest tests/deploy/test_deploy_script.py tests/deploy/test_helm_chart.py -q`".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/docs/adr/0070-restart-safe-live-sessions.md`:
- Around line 84-100: The pytest command at line 100 in the acceptance criteria
has incorrect file paths that lack the `coding-agent/` prefix required for this
monorepo structure. Update the command to use the correct paths relative to the
repository root: change `tests/coding_agent/test_session_restart_durability.py
tests/deploy/test_helm_chart.py` to
`coding-agent/tests/coding_agent/test_session_restart_durability.py
coding-agent/tests/deploy/test_helm_chart.py`. Alternatively, you may preface
the command with an instruction to execute from the `coding-agent/` directory.
The `-k "restart or drain or rehydrate"` filter pattern remains correct and
safe.
---
Outside diff comments:
In `@coding-agent/docs/adr/0071-argocd-gitops-deployment.md`:
- Around line 1-105: In the Acceptance Criteria section, two corrections are
needed: First, on line 84, replace the hardcoded release name
`coding-agent-deepseek` with `coding-agent` to match the defaults in the deploy
script (where both HELM_RELEASE and K8S_NAMESPACE default to `coding-agent`),
unless this is an environment-specific override that should be clarified in the
criterion text. Second, on lines 92-93, the final acceptance criterion is
incomplete and consists only of a bare test command; rephrase it as a proper
criterion description (e.g., starting with an action verb and outcome) that
includes the test command, such as "Existing deploy-script and helm-chart tests
pass: `uv run pytest tests/deploy/test_deploy_script.py
tests/deploy/test_helm_chart.py -q`".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 463ab193-5337-4a7e-9183-63248ec63296
📒 Files selected for processing (2)
coding-agent/docs/adr/0070-restart-safe-live-sessions.mdcoding-agent/docs/adr/0071-argocd-gitops-deployment.md
Independent code review (and codex re-review) found ADR-0070's original P1 premise wrong: POST /sessions already persists synchronously at create (session_manager.py:2754), and neither shutdown nor startup deletes session rows. The real cause of the lost durable session row is the 30-min idle GC: cleanup_idle_sessions -> close_session -> _remove_session_async -> delete_session destructively deletes durable sessions. Restart-independent. Rewrite 0070 around that: D1 (primary) idle GC must only shut down runtime + release lease, never delete durable metadata/tape/runs/checkpoints — destructive delete only on explicit user/admin close. P2 kept as in-flight-turn graceful- drain hardening (not the row-loss cause). P3 downgraded to optional runtime rehydration/resume (no promise to recover a killed turn process). Drop the create-time-persistence proposal (already implemented). Minor 0071 fixes: dependency wording -> 'D1 idle-GC fix + D2 graceful drain'; replace the nonexistent test_deploy_script_fails_closed_on_placeholder_values with the real apply-mode placeholder-rejection test names. Docs only; no runtime code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
delete-cloud
changed the title
- 0070: restart surfaces (not immediately triggers) the loss — cleanup task sleeps 60s before first pass. - 0070 D1: add ownership note — if idle cleanup releases the owner lease, the next prompt must reacquire (current path only _assert_owners). - 0070 D3: resume is best-effort from durable history/context per ADR-0055, not a checkpoint rollback (was overstated). - 0070 acceptance: fix test path tests/coding_agent -> tests/ui. - 0071: separate the idle-GC row-loss cause (D1) from the drain gap (D2); drop the claim that manual SSH 'caused' the session loss (idle GC did). Docs only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Clarify the ArgoCD sync namespace: coding-agent-deepseek is the o6n-prod override (chart/deploy-script default is coding-agent), not a hardcoding error. - Rephrase the bare pytest command into a full acceptance-criterion sentence. Docs only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 16, 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
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.
Two Proposed ADRs. Revised after an independent + codex code review corrected ADR-0070's root cause.
ADR-0070 — Durable session retention and graceful drain
Corrected root cause: the REVISION 6 rollout appeared to lose sessions, but create-time persistence already exists (
POST /sessionspersists synchronously atsession_manager.py:2754) and neither shutdown nor startup deletes rows. The real deleter is the 30-min idle GC:cleanup_idle_sessions→close_session→_remove_session_async→delete_sessiondestructively deletes durable sessions, restart-independent.Decision:
terminationGracePeriodSeconds) as in-flight-turn hardening — not the row-loss cause.ADR-0071 — ArgoCD GitOps deployment
Unchanged in substance (pure ArgoCD, private values repo, immutable
:<sha>, manual sync). Minor fixes: dependency wording → "D1 idle-GC fix + D2 graceful drain"; replaced a nonexistent test name with the real apply-mode placeholder-rejection tests.Implementation will land as separate PRs after this docs PR merges — a #607 runtime-config atomicity hotfix (separate), then D1, then D2, then ArgoCD (after D1/D2 are live). This PR is docs only.
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Infrastructure & Reliability