Repository navigation
Backend: replay a capture after client restart (cold restart across process death) - #267
Conversation
A device that dies mid-capture must recover from what it persisted and what the server remembers — not from its own cached state. PR #262's replay resumed a lost response inside one Python process: the client that retried was the interpreter that lost the reply. scripts/capture_cold_restart.py splits the client across two fresh Python processes against a real create_app() server (memory state, local volume storage, orchestrator stage stubs — no network, no credentials): - the primary client replays a saved capture_flow_e2e receipt and dies at a scripted interruption: a lost create/commit/finalize response or a stop before commit/finalize, persisting only a client journal of what it sent and which replies it saw. - the resume client reads the receipt and journal and recovers through the real routes: create again with the saved idempotency key, then register-upload-commit-finalize — or, when the dead process already finalized, straight through the finalize's idempotent replay, because the capture is closed to files and commits (the server 409s both). Across the restart it asserts one capture id (a 200 replay, never a second 201, still mapping to the same capture after the verdict), one deterministic run id (run_ + sha256 of captureId, NUL and the packet bytes, unchanged on the second finalize), committed hashes matching the receipt with no mismatch, and one terminal state (one terminal status, /result served once for that run, exactly one verdict_ready event). Every wait is bounded: server readiness, per-leg subprocess timeouts, per-request timeouts, and capped terminal/event polling. A receipt written before the replay fields existed fails before anything is sent, naming the missing field. tests/test_capture_cold_restart.py drives the CLI as a subprocess for all five interruption points, asserts the two legs ran as different fresh processes sharing only the receipt, journal and server, and checks pre-replay-field receipts fail loudly naming each missing field. Co-authored-by: Sam Gu <sgu07966@gmail.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Independent review (Obvious, Agent fallback — the Autobuild card was declined, so this ran as Agent) Verdictready — still a draft; nothing here blocks ready-for-review. No blocking findings; the PR does what the body claims, and every claim I could execute reproduced. What the PR isTwo new files, nothing else: What I verified (commands and results)
Mutation checks — the new tests biteEach mutation was applied, tested, then reverted (tree clean at
Findings (ranked; none blocking)
Continuity note (positive): the What I couldn't verify
I changed no code; this comment is my only write to the PR. |
|
Note Review skipped Codemetrics is installed on this repository, but it hasn't been added to your team yet. To enable AI code reviews for this repo, go to your dashboard and add BaseScanning to your team's repositories. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] New test scripts for client recovery after process restart. Disable inherited remote Modal execution before merging so this local check cannot start remote work.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "--receipt", | ||
| str(receipt_path), | ||
| "--workdir", | ||
| str(workdir), | ||
| "--server-url", | ||
| server_url, | ||
| *extra, |
There was a problem hiding this comment.
--packet is checked in the parent but never passed to either client process. If a saved fixture has moved and the user supplies its new location, the parent accepts it, but both clients fall back to the old location or the default fixture and fail. Pass the resolved packet directory to both legs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_cold_restart.py
Line: 782-788
Comment:
**Clients ignore the packet override**
`--packet` is checked in the parent but never passed to either client process. If a saved fixture has moved and the user supplies its new location, the parent accepts it, but both clients fall back to the old location or the default fixture and fail. Pass the resolved packet directory to both legs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| server_url, | ||
| *extra, | ||
| ], | ||
| cwd=str(ROOT), |
There was a problem hiding this comment.
Relative --receipt and --workdir paths change meaning when the clients start with cwd=ROOT. Running from the repository root with --receipt ./saved/receipt.json --workdir ./restart makes the clients look under backend/, while the parent uses the repository root. Valid paths then produce missing-receipt or missing-report errors. Resolve these paths before starting any child process.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_cold_restart.py
Line: 790
Comment:
**Relative paths point elsewhere**
Relative `--receipt` and `--workdir` paths change meaning when the clients start with `cwd=ROOT`. Running from the repository root with `--receipt ./saved/receipt.json --workdir ./restart` makes the clients look under `backend/`, while the parent uses the repository root. Valid paths then produce missing-receipt or missing-report errors. Resolve these paths before starting any child process.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| def _child_env() -> dict: | ||
| env = os.environ.copy() | ||
| env["PYTHONPATH"] = str(ROOT) + (os.pathsep + env["PYTHONPATH"] if env.get("PYTHONPATH") else "") | ||
| # The orchestrator reads this from the environment when the API binds it. | ||
| env["HSS_ORCH_STAGE_STUBS"] = "1" | ||
| return env |
There was a problem hiding this comment.
If the invoking shell has HSS_ORCH_MODAL_REMOTE=1 and Modal is installed, _child_env() passes that switch to the server. HSS_ORCH_STAGE_STUBS=1 does not block it: the coordinator calls launch_modal() before the local stub executor. This can start paid work on the deployed app or fail for missing credentials. Force HSS_ORCH_MODAL_REMOTE=0 for this local command.
| def _child_env() -> dict: | |
| env = os.environ.copy() | |
| env["PYTHONPATH"] = str(ROOT) + (os.pathsep + env["PYTHONPATH"] if env.get("PYTHONPATH") else "") | |
| # The orchestrator reads this from the environment when the API binds it. | |
| env["HSS_ORCH_STAGE_STUBS"] = "1" | |
| return env | |
| def _child_env() -> dict: | |
| env = os.environ.copy() | |
| env["PYTHONPATH"] = str(ROOT) + (os.pathsep + env["PYTHONPATH"] if env.get("PYTHONPATH") else "") | |
| # The orchestrator reads these from the environment when the API binds it. | |
| env["HSS_ORCH_STAGE_STUBS"] = "1" | |
| env["HSS_ORCH_MODAL_REMOTE"] = "0" | |
| return env |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_cold_restart.py
Line: 685-690
Comment:
**Local checks launch remote work**
If the invoking shell has `HSS_ORCH_MODAL_REMOTE=1` and Modal is installed, `_child_env()` passes that switch to the server. `HSS_ORCH_STAGE_STUBS=1` does not block it: the coordinator calls `launch_modal()` before the local stub executor. This can start paid work on the deployed app or fail for missing credentials. Force `HSS_ORCH_MODAL_REMOTE=0` for this local command.
```suggestion
def _child_env() -> dict:
env = os.environ.copy()
env["PYTHONPATH"] = str(ROOT) + (os.pathsep + env["PYTHONPATH"] if env.get("PYTHONPATH") else "")
# The orchestrator reads these from the environment when the API binds it.
env["HSS_ORCH_STAGE_STUBS"] = "1"
env["HSS_ORCH_MODAL_REMOTE"] = "0"
return env
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| journal["finalizeExecuted"] = True | ||
| if stop == "finalize-lost": | ||
| record("finalize", responseLost=True) | ||
| return _finish(report, errors, None) |
There was a problem hiding this comment.
The commit-lost and finalize-lost branches return success before checking whether the server accepted the request. A rejected request can therefore be followed by the resume client's first successful operation, while the report claims it proved replay after a lost successful response. Check the primary request's status and commit results before discarding its reply, without copying that reply into the device journal.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/scripts/capture_cold_restart.py
Line: 379-382
Comment:
**Rejected requests count as replay**
The `commit-lost` and `finalize-lost` branches return success before checking whether the server accepted the request. A rejected request can therefore be followed by the resume client's first successful operation, while the report claims it proved replay after a lost successful response. Check the primary request's status and commit results before discarding its reply, without copying that reply into the device journal.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Human author: Sam Gu (sgu07966@gmail.com)
Summary
PR #262 closes the loop on a lost response, but its resume runs inside one Python process: the client that retries is the same interpreter that lost the reply, so it still holds whatever it cached. A phone does not get that luxury — the app dies, relaunches, and must recover from what it persisted and what the server remembers.
This PR adds
backend/scripts/capture_cold_restart.py: one repeatable command that splits the client across two fresh Python processes against a realcreate_app()server (local uvicorn, memory state, local volume storage, the documented dev handoffHSS_INGEST_HOOKS=autowith orchestrator stage stubs — no GCP, no Modal, no network, no credentials):capture_flow_e2ereceipt (create, register, PUT, commit, finalize) and dies at a scripted interruption: a lostcreate/commit/finalizeresponse (*-lost) or a stop before commit/finalize (pre-*). It persists only what a device could: a journal of what it sent and which replies it actually saw.Across the restart it asserts the things a raw HTTP log cannot prove:
run_+ sha256(captureId + NUL + packet bytes), unchanged on a second finalize, recovered even when the dead process never saw it;/resultserved once for that run with the matching outcome, and exactly one terminalverdict_readyevent.Nothing retries forever: server readiness, per-leg subprocess, per-request, and terminal/event polling are all bounded. A receipt written before the replay fields existed fails before anything is sent, naming the missing field.
Test evidence
backend/tests/test_capture_cold_restart.py(14 tests):committed, identical commit body commits again); finalize-lost recovers the run id the dead process never saw via the finalize-replay path; create-lost recovers a capture id that never reached the client; pre-commit uploads and commits for the first time;idempotencyKeys,files,packetSha256) before anything reaches the API — in-process and via the CLI exit code.Verified in this branch's environment:
uv run --frozen --extra api pytest tests/test_capture_cold_restart.py tests/test_replay_capture.py tests/test_capture_flow_e2e_runner.py -q— 23 passed;ruff format --checkandruff check— clean;lint-imports— all contracts pass.Stacked on #262 (
obv/burst5-20261015-bs-capture-replay); GitHub retargets it automatically when #262 merges.Risk / rollout
.github/, or deployment;replay_capture.py's owned files are untouched.backend/scripts/capture_cold_restart.py,backend/tests/test_capture_cold_restart.py.🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact