fix(storage): harden #558 satellite backup writes and zst restore - #571
Conversation
…store Make every satellite-backup.json update crash-safe (tmp + fsync + rename + best-effort dir fsync) so a failed post-memories rewrite cannot truncate the last valid snapshot. Restore legacy compressed-only quarantine entries by decompressing a lone .jsonl.zst in memory and reconstructing the thread row.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughArchived cleanup now atomically replaces satellite backups with crash-safety hooks, while legacy restore reconstructs thread metadata from compressed ChangesArchived cleanup and restore
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b11dedeadf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const payload = JSON.stringify(backup); | ||
| const fd = openSync(tmp, "w", 0o600); | ||
| try { | ||
| writeSync(fd, payload, null, "utf8"); |
There was a problem hiding this comment.
When the filesystem returns a short write, which can occur for a large backup under disk or resource pressure, this ignores the byte count returned by writeSync, fsyncs the incomplete temp file, and renames it over the last valid satellite-backup.json. The resulting truncated JSON defeats the crash-recovery guarantee this change is intended to provide; loop until the entire payload is written, as appendRolloutLine already does, before fsync and rename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ba53cc0: write loop drains short writes before fsync, and rename uses renameAtomicFile (Windows EBUSY/EPERM/EACCES retries).
| throw new Error("test_fail_satellite_backup_replace"); | ||
| } | ||
| try { | ||
| renameSync(tmp, dest); |
There was a problem hiding this comment.
On Windows, antivirus or another scanner can briefly hold the existing backup and make this replacement fail with EBUSY, EPERM, or EACCES; the code then aborts an otherwise valid cleanup after committing satellite mutations and runs the recovery path. The repository already provides renameAtomicFile, including bounded retries for exactly these Windows sharing violations, so use that helper or equivalent retry logic for this atomic replacement.
AGENTS.md reference: AGENTS.md:L36-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ba53cc0 via renameAtomicFile for the satellite-backup rename.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/server/management/logs-usage-routes.ts`:
- Around line 365-371: Add focused regression tests for the guarded test-stream
route in the routing test suite. Verify GET
/api/storage/trash/restore/test-stream returns a JSON 404 when
OPENCODEX_CLEANUP_TEST_HOOKS is unset or disabled and when
getRestoreTrashTestStreamResponse() returns null, ensuring it does not fall
through to GUI HTML; also verify the enabled hook returns the stream response.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 8075390b-fa0d-42ea-b850-d0cf9003e9ab
📒 Files selected for processing (4)
src/codex/history-provider.tssrc/server/management/logs-usage-routes.tssrc/storage/cleanup.tstests/storage-cleanup.test.ts
| if (url.pathname === "/api/storage/trash/restore/test-stream" && req.method === "GET") { | ||
| if (process.env.OPENCODEX_CLEANUP_TEST_HOOKS === "1") { | ||
| const stream = getRestoreTrashTestStreamResponse(); | ||
| if (stream) return stream; | ||
| } | ||
| // Always answer this test-only path — never fall through to the GUI SPA (200 HTML). | ||
| return jsonResponse({ error: "not_available" }, 404); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add focused regression coverage for the guarded test-stream route.
This change must verify that GET /api/storage/trash/restore/test-stream returns JSON 404—not the GUI HTML—when OPENCODEX_CLEANUP_TEST_HOOKS is unset/disabled and when getRestoreTrashTestStreamResponse() returns null. Also cover the enabled-stream path so future routing changes cannot bypass the test hook.
🤖 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 `@src/server/management/logs-usage-routes.ts` around lines 365 - 371, Add
focused regression tests for the guarded test-stream route in the routing test
suite. Verify GET /api/storage/trash/restore/test-stream returns a JSON 404 when
OPENCODEX_CLEANUP_TEST_HOOKS is unset or disabled and when
getRestoreTrashTestStreamResponse() returns null, ensuring it does not fall
through to GUI HTML; also verify the enabled hook returns the stream response.
Source: Path instructions
There was a problem hiding this comment.
CodeRabbit coverage follow-up: env-unset JSON 404 was already in #571; remaining null-hook + enabled-stream cases are in a new PR targeting dev (test-stream gate regressions).
There was a problem hiding this comment.
@Wibias, acknowledged. The env-unset JSON 404 regression is already covered in #571, including the JSON response behavior at src/server/management/logs-usage-routes.ts Lines 365-371.
The two remaining branches—hook enabled with getRestoreTrashTestStreamResponse() returning null, and hook enabled with a stream response—are appropriately tracked in the dedicated dev-targeting follow-up PR. I’ll leave this finding unresolved on #571 rather than treating deferred coverage as completed here.
…follow-up Loop short writes to completion and rename via renameAtomicFile so Windows sharing violations cannot replace a durable temp with a truncated backup.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
* test(storage): cover restore test-stream route gate cases Address CodeRabbit on #571: assert JSON 404 when the stream hook is null under OPENCODEX_CLEANUP_TEST_HOOKS=1, and that the enabled stream path returns the text/plain chunked response. * test(storage): isolate restore test-stream route env and hooks Reset OPENCODEX_CLEANUP_TEST_HOOKS and restore-job hooks in a nested finally so enableTestStream cannot leak across cases in the responsive suite.
Summary
Follow-up fixes for already-merged PR #558 (Phase 2.1 quarantine restore).
Bugs
satellite-backup.jsonwrites —writeSatelliteBackupused plainwriteFileSync, so a crash during the post-memories rewrite (after a satellite DB commit) could truncate or destroy the last valid backup and leave recovery impossible..jsonl.zst(no plain sibling, nosatellite-backup.jsonthread snapshot) could not reconstruct production-shapedthreadsrows because restore resolved the logical.jsonlpath and refused to parse zstd.Implementation
Crash-safe satellite backup writes
fsyncthe temp, atomicallyrenameoversatellite-backup.jsonfsyncwhere supportedfailSatelliteBackupReplacetest hook fails after durable temp write / before renameLegacy
.jsonl.zstrestorerolloutPathis missingreadThreadFieldsFromRolloutdecompresses.zstin memory withzstdDecompressSync({ maxOutputLength })(64 MiB cap)Also: test-only
/api/storage/trash/restore/test-streamnow returns JSON 404 when the cleanup test-hooks env is off, instead of falling through to the GUI SPA (200 HTML) — keeps the #558 responsive-suite gate deterministic.Test plan
.jsonl.zst→ restore recovers compressed file and correct thread rowbun run typecheckSummary by CodeRabbit
.jsonl.zst), including legacy quarantine scenarios.satellite-backupsnapshots when cleanup encounters a failure.