Uh oh!
There was an error while loading. Please reload this page.
feat(durability): productionize DBOS — entrypoint wiring, resume proof, validated procedure (#154) - #474
Conversation
…f, validated procedure (#154) ADR 0011 shipped the durable shim off + unvalidated: main.py never constructed/launched DBOS (the shim 'trusted' an init that didn't exist), the DBOS_DATABASE_URL precondition was docstring-only, and nothing tested either mode. - services/durable.py: enforce the DATABASE_URL precondition at activation; new init_dbos()/shutdown_dbos() — construct + DBOS.launch() from the lifespan, fail-loud when the operator explicitly opted in (#174 posture). Decorate-before-construct-before-launch order verified against dbos==2.28.0's registry internals. - main.py: init_dbos() after validate_config(), shutdown_dbos() on teardown. - requirements-durable.txt: the opt-in extra ADR 0011 promised (dbos>=2.28,<3), never in requirements.txt/lock. - tests/test_durable_shim.py: 11 hermetic tests over both modes and every precondition combination (fake dbos module + reload, pristine restore). - tests/test_dbos_resume.py: opt-in (RUN_DBOS_RESUME=1) subprocess crash/ resume proof — step1 runs EXACTLY once across an os._exit crash and the workflow completes on relaunch recovery — plus process_document parity under real DBOS. Exercised against dbos 2.28 + a real Postgres. - test_documents_routes.py: streaming replay now also pins zero re-inserts (the #132 crash-after-result scenario, with the #464 exactly-one-result guard). - ADR 0011 → accepted (shipped + validated, default off): corrected activation procedure (launch() migrates + auto-recovers; 'dbos migrate' was stale), resume monitoring (startup INFO line -> Logfire per #119, workflow_status SQL), streaming-asymmetry reaffirmed. Closes#154 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | cd637c4 | Commit Preview URL Branch Preview URL | Jul 30 2026, 04:09 PM |
Warning Review limit reached
Next review available in:21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughDBOS lifecycle handling is added to the FastAPI lifespan, with configuration-gated decorators and opt-in dependency installation. Hermetic tests cover shim modes and failures; subprocess tests cover crash recovery and output parity. Streaming replay tests verify no duplicate document writes, and architecture documentation is updated. ChangesDurable execution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant FastAPI
participant services.durable
participant DBOS
participant Postgres
FastAPI->>services.durable: init_dbos()
services.durable->>DBOS: construct with system_database_url
DBOS->>Postgres: launch and access DBOS state
FastAPI-->>FastAPI: serve requests
FastAPI->>services.durable: shutdown_dbos()
services.durable->>DBOS: destroy()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/main.py (1)
85-97: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
init_dbos()failure leaks the events-service worker thread.
init_dbos()is documented to raise whenDBOS_ENABLED=trueand construction/launch fails. It's called at line 92, afterevents_service.start_worker()(line 88) but beforeyield(line 93). If it raises, the generator never reachesyield, soevents_service.shutdown()(line 96) andshutdown_dbos()(line 97) never run — the drain thread and any queued rows are abandoned on the failed-startup path, contradicting the "last batch... isn't lost" intent of the shutdown comment.Reorder so
init_dbos()runs before starting the events worker, or wrap the sequence to flush/stopevents_serviceon failure before re-raising.🔧 Suggested reorder
- from services import events_service- events_service.start_worker()- # ADR 0011 / `#154`: construct + launch DBOS when DBOS_ENABLED=true; no-op- # passthrough otherwise. Fails loudly (raises) if the operator opted in- # and launch fails — see services/durable.py::init_dbos.- init_dbos()+ # ADR 0011 / `#154`: construct + launch DBOS when DBOS_ENABLED=true; no-op+ # passthrough otherwise. Fails loudly (raises) if the operator opted in+ # and launch fails — see services/durable.py::init_dbos. Run before+ # starting the events worker so a failed launch doesn't leak that thread.+ init_dbos()+ from services import events_service+ events_service.start_worker() yield🤖 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 `@backend/main.py` around lines 85 - 97, Reorder the startup sequence in the application lifespan flow so init_dbos() completes before events_service.start_worker() is called. Preserve the existing yield and shutdown ordering, ensuring a DBOS initialization failure occurs before the events worker starts and cannot leak that thread.
🤖 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 `@docs/decisions/0011-durable-execution-dbos.md`:
- Around line 44-50: Update docs/decisions/0011-durable-execution-dbos.md lines
44-50 to describe test_pipeline_identical_with_dbos_on as asserting parity for
category, abstract, and concept names, rather than byte-for-byte output
equality. Update lines 117-122 to state that classifier_agent.run is patched
with an AsyncMock and asserted not called, replacing “unmocked-but-spied”
wording.
- Around line 26-29: Update the `backend/requirements-durable.txt` description
so `dbos>=2.28,<3` is identified as a tested compatible range rather than
pinned; alternatively, change the requirement to an exact `dbos==2.28.0` pin and
retain the pinned wording.
---
Outside diff comments:
In `@backend/main.py`:
- Around line 85-97: Reorder the startup sequence in the application lifespan
flow so init_dbos() completes before events_service.start_worker() is called.
Preserve the existing yield and shutdown ordering, ensuring a DBOS
initialization failure occurs before the events worker starts and cannot leak
that thread.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22eba8b8-500e-4aef-81fa-affa1d8cc3ac
📒 Files selected for processing (8)
backend/main.pybackend/requirements-durable.txtbackend/services/durable.pybackend/tests/test_dbos_resume.pybackend/tests/test_documents_routes.pybackend/tests/test_durable_shim.pydocs/architecture.mddocs/decisions/0011-durable-execution-dbos.md
| 3. **`backend/requirements-durable.txt` now exists** (`dbos>=2.28,<3`, | ||
| pinned to the version this update's verification ran against). Never | ||
| added to `requirements.txt`/`requirements.lock` — durability stays | ||
| opt-in and the hermetic suite runs with `dbos` NOT installed. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not describe a compatible-version range as pinned.
dbos>=2.28,<3 permits later 2.x releases; it is not pinned to 2.28.0. Either use dbos==2.28.0 or describe this as a tested compatible range.
🤖 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 `@docs/decisions/0011-durable-execution-dbos.md` around lines 26 - 29, Update
the `backend/requirements-durable.txt` description so `dbos>=2.28,<3` is
identified as a tested compatible range rather than pinned; alternatively,
change the requirement to an exact `dbos==2.28.0` pin and retain the pinned
wording.
| `test_pipeline_identical_with_dbos_on` runs | ||
| `agents.document.process_document` for real under DBOS | ||
| (`SAPLING_MODEL_MODE=function`) and asserts its output matches the | ||
| function-mode constants byte-for-byte — proof that wrapping the | ||
| pipeline in `@durable_workflow`/`@durable_step` changes durability, | ||
| not behavior. Both tests were run against a real `dbos==2.28.0` + | ||
| a throwaway Postgres during this update and passed; see the file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the ADR’s test claims with the actual assertions.
docs/decisions/0011-durable-execution-dbos.md#L44-L50: describe this as parity for category, abstract, and concept names, or expand the test to compare the complete result; it does not currently prove byte-for-byte output equality.docs/decisions/0011-durable-execution-dbos.md#L117-L122: replace “unmocked-but-spied” with wording thatclassifier_agent.runis patched with anAsyncMockand asserted not called.
📍 Affects 1 file
docs/decisions/0011-durable-execution-dbos.md#L44-L50(this comment)docs/decisions/0011-durable-execution-dbos.md#L117-L122
🤖 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 `@docs/decisions/0011-durable-execution-dbos.md` around lines 44 - 50, Update
docs/decisions/0011-durable-execution-dbos.md lines 44-50 to describe
test_pipeline_identical_with_dbos_on as asserting parity for category, abstract,
and concept names, rather than byte-for-byte output equality. Update lines
117-122 to state that classifier_agent.run is patched with an AsyncMock and
asserted not called, replacing “unmocked-but-spied” wording.
…doc claims
- /upload/sync now pins the DBOS workflow id to doc:{user_id}:{request_id}
(user-scoped — X-Request-ID is client-supplied), so a client retry
attaches to the SAME workflow: completed -> recorded result, crashed ->
resume at last completed step. Graph merge wrapped as _step_apply_graph
so resume never re-runs the one real side effect.
- init_dbos now RAISES on flag-on-but-preconditions-unmet (missing URL /
failed import) instead of silently degrading an explicit opt-in; shim
tests pin both raise paths.
- Doc corrections: pre-#154 flag-on raised DBOSException per call (502s),
not 'silently nothing'; step-outside-workflow runs the plain function on
dbos 2.28 (not undefined); only database_url is deprecated.
- New opt-in proof test_pipeline_crash_resume_via_workflow_id: real
process_document crash mid-_run_workers -> same-id retry -> classify
step runs EXACTLY once across both phases. All 3 resume tests exercised
green against dbos 2.28 + a real Postgres.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>AndresL230
commented
Jul 30, 2026
Code reviewNo issue scored above the posting threshold (five review passes: CLAUDE.md compliance, shallow bug scan, git history + code-comment compliance, prior-PR comments; scored 25–75). All six sub-threshold findings were fixed anyway in cd637c4:
3–4. Doc-accuracy: the ADR/docstring claimed the pre-#154 flag-on state "silently did nothing" (it actually raised The remaining pass observation — no pre-merge e2e-lane run reported yet — is the next pipeline stage; the full local cycle (Playwright + oracles under the stack lock) runs before merge and results will be posted here. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
AndresL230
commented
Jul 30, 2026
Pre-merge local e2e cycle at cd637c4 under the stack lock (SAPLING_MODEL_MODE=function): |
Closes#154. Last item in the agent-migration epic #152.
ADR 0011 shipped the durable shim off + unvalidated:
main.pynever constructed or launched DBOS (the shim's import-time activation "trusted that DBOS() was called in main.py" — no such call existed), theDBOS_DATABASE_URLprecondition was docstring-only,requirements-durable.txtwas promised but never created, and neither mode had any test coverage.What this does
services/durable.py: activation now requiresDBOS_ENABLED=true+ importabledbos+ non-emptyDBOS_DATABASE_URL(code matches its docstring). Newinit_dbos()/shutdown_dbos()called frommain.py's_lifespan: construct the singleton (system_database_urlconfig — the deprecateddatabase_urlkey provisions a separate app DB used only by@DBOS.transaction, which we don't use) andDBOS.launch(). Fail-loud contract: any construct/launch failure under an explicit opt-in raises at startup (No startup env validation + silent HMAC-less SESSION_SECRET fallback #174 posture) — a silent passthrough would look durable while resuming nothing. Decorate-before-construct-before-launch verified against dbos==2.28.0 internals (registrations land on a lazily-created global registry the laterDBOS.__init__picks up).requirements-durable.txt: the opt-in extra (dbos>=2.28,<3), deliberately outside requirements.txt/lock.tests/test_durable_shim.py(11 hermetic tests, no dbos needed): passthrough parity + every precondition combination via env-patched reload of a fakedbosmodule, with pristine-state restore so the already-importedagents.documentdecorations are untouched.tests/test_dbos_resume.py(opt-in,RUN_DBOS_RESUME=1): subprocess crash/resume proof — step 1 checkpoints, step 2os._exit(42)s the process, relaunch auto-recovers the PENDING workflow, and step 1's side-effect counter shows exactly one invocation (resume at last completed step). Plusprocess_documentparity under real DBOS in function mode. Exercised twice against dbos 2.28 + a real throwaway Postgres (implementer + an independent re-run): 2 passed in 3.56s.result(agents already pinned as not re-run; the fix(backend): close the #136 contract-audit tail + #340 + #72 #464 guard keeps exactly-one-result).DBOS.launch()runs system-DB migrations itself and auto-recovers PENDING workflows — the olddbos migratestep was stale), resume monitoring (startup INFO line captured by Logfire per [P2] Observability: activate Logfire for ops/error/LLM tracing #119 +dbos.workflow_statusSQL), and the intentional streaming-route asymmetry reaffirmed. DBOS's system schema is DBOS-managed, deliberately outsidedb/migrations/.Gates
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation