Skip to content

fix(storage): publish cleanup manifests atomically - #3786

Merged
lidge-jun merged 4 commits into
devfrom
codex/track3-storage-manifests-a4b0
Sep 6, 2026
Merged

fix(storage): publish cleanup manifests atomically#3786
lidge-jun merged 4 commits into
devfrom
codex/track3-storage-manifests-a4b0

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • Publish cleanup manifests through the same private, file-synced, atomic replacement path as satellite backups. A handled failure before replacement retains the previous complete recovery record instead of truncating it.
  • Add real-filesystem regression cases for initial publication, pre-delete replacement, all-purge-failed restoration and partial-purge fail-closed behavior. Existing restore and database ordering remain unchanged.
  • Closes [Bug]: Non-atomic cleanup manifest writes can strand archived sessions after a crash #3778, reported by @turin-dev.

Track 3 manual stack, layer 1 (base: dev). Later layers add separate Docker persistence, access-key recipes, Windows diagnostic origins and create-only configuration initialization. This is an ordinary branch chain, not a registered native stack.

Verification

  • Independent Astra high correctness/security review: PASS, no blocking findings. It inspected the publication helper, callback isolation, restore behavior, regression oracles and documentation.
  • git diff --cached --check passed. No local tests, typecheck, builds or suite were run, as explicitly instructed by the maintainer.
  • Runtime validation: the cumulative exact-source CI evidence is recorded below; lower-layer tests are deferred under the explicit maintainer policy.
  • Exception-injection tests are not physical power-loss proof. Directory fsync remains best-effort and partial permanent purge is not made reversible.

Cumulative tested source: 9ea896737bd17085ebb8a69b3320c9cace61889e, based on dev eff908e0fb9452d5ff2952af1c5dafc1c4c35dd9.

  • Cross-platform CI: all 25 jobs successful. Attempt 1 failed only the existing Windows cli-models first-child 15-second watchdog. One unchanged-head rerun of that shard completed 3,489 cases (3,465 pass, 24 skip, 0 fail); the first models case took 943 ms. The initial failure is retained; the slow phase remains unproven and no flakiness fix is claimed.
  • Service lifecycle: Linux, macOS and Windows passed on the same source head.
  • Documentation build: passed after checking out and asserting that exact source. Its isolated workflow-definition commit is different and is not part of this stack; no deployment occurred.
  • Independent scoped code/security and integration reviews: PASS. The two earlier Windows fixture-budget changes retain production behavior and original assertions, backed by readiness controls and quota restart controls. Diagnostic workflows, instrumentation and mutations are excluded from delivery.

The maintainer explicitly authorizes dev admin integration and final-head-first validation for this manual chain: #3786#3788#3789#3790#3796#3802. Lower automatic tests are deferred with [skip ci], not represented as passing. No local tests, typecheck, builds or suites were run; pushes used --no-verify. Merge commits preserve source ancestry and suppress duplicate lower CI; fresh head/base/actor/review checks and resulting tree checks accompany each merge.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Landing confirmed: merged into dev as 43970794feef5984858ed2afec345861ca227e39. The final chain merge is f89b815090020f52089801b50f569a777bdfdc0a; its tree 846b8ffc383fc0ba99486917bdfb820a0fa432ba equals the tested cumulative source. Source ancestry and every merge were verified after fetching dev.

Preserve complete recovery records across handled publication failures and keep existing partial-purge restoration boundaries. Refs #3778. Local checks deferred to final stack CI by maintainer instruction.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 679d1403-3628-4cd6-98eb-c89366c1691f

📥 Commits

Reviewing files that changed from the base of the PR and between eff908e and d682e06.

📒 Files selected for processing (4)
  • docs-site/src/content/docs/reference/management-api.md
  • src/storage/cleanup.ts
  • structure/02_config-and-codex-home.md
  • tests/storage/storage-cleanup.test.ts

📝 Walkthrough

Walkthrough

Cleanup manifests and satellite backups now use atomic temporary-file publication. Tests cover staging, replacement, and purge-incomplete failures. Documentation records prior-manifest preservation and restore behavior after permanent purges.

Changes

Cleanup publication

Layer / File(s) Summary
Atomic writer and backup integration
src/storage/cleanup.ts
writePrivateFile creates, writes, fsyncs, chmods, and atomically renames a temporary file. Satellite backups use this path.
Manifest publication phases
src/storage/cleanup.ts
Manifest writes report staging, pre-commit, and purge-incomplete phases through a test-only hook.
Failure recovery validation and documentation
tests/storage/storage-cleanup.test.ts, structure/02_config-and-codex-home.md, docs-site/src/content/docs/reference/management-api.md
Tests verify temporary cleanup, prior-manifest preservation, database consistency, staged-file retention, permissions, and restore outcomes. Documentation describes atomic publication and fail-closed restore behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Cleanup
  participant writePrivateFile
  participant TemporaryFile
  participant Manifest
  participant restoreTrashEntry
  Cleanup->>writePrivateFile: publish cleanup manifest
  writePrivateFile->>TemporaryFile: write and fsync complete contents
  writePrivateFile->>Manifest: atomically replace target
  restoreTrashEntry->>Manifest: read published manifest
  Manifest-->>restoreTrashEntry: return valid recovery record
  restoreTrashEntry->>Cleanup: restore staged files when records remain valid
Loading
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/track3-storage-manifests-a4b0

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 68 / 80

지금 dev HEAD(24c761a05, package 2.45.0)의 src/storage/cleanup.ts를 보면, cleanup 복구 매니페스트를 쓰는 writePrivateFile(대략 118행 근처)이 그냥 writeFileSync로 목적 파일을 덮어씁니다. 같은 파일 안의 writeSatelliteBackup은 이미 임시 파일 → fsyncrenameAtomicFile 경로를 쓰고 있어서, 크래시나 교체 실패가 나도 마지막 온전한 백업이 잘리지 않습니다. 이 PR은 그 불일치를 없애서, 매니페스트도 위성 백업과 같은 “완전 기록 후 원자 교체”로 맞춥니다. 이슈 #3778이 말한 바로 그 구멍입니다. 아카이브 정리 중에 프로세스가 죽으면, 반쯤 쓴 매니페스트 때문에 복구 목록이 깨지고 세션이 떠돌 수 있습니다.

코드 쪽으로는 writePrivateFilepath.pid.uuid.tmpwx로 만들고, 내용을 쓴 뒤 파일 fsync, chmod, 선택 훅, 그다음 renameAtomicFile(이 PR에서는 ../lib/windows-atomic-replace에서 직접 import)로 바꿉니다. 교체 전에 실패하면 예전 목적 파일이 그대로 남습니다. writeSatelliteBackup도 같은 헬퍼를 쓰도록 줄였고, beforeManifestReplace 훅은 wire 직렬화 목록에서 빼서 관리 API로 함수가 흘러가지 않게 했습니다. tests/storage/storage-cleanup.test.ts에 최초 게시, 교체 직전 실패, purge 전부 실패 복원, 부분 purge fail-closed 같은 실제 파일시스템 회귀가 붙었습니다. docs-site management-api와 structure/02_config-and-codex-home.md에도 “원자 게시는 영구 purge를 되돌리지 않는다”는 한계가 적혀 있습니다.

Track 3 수동 스택의 맨 아래 층(base=dev)이고, 위층 #3788·#3789가 이 브랜치에 얹힙니다. types/config 분할 캠페인과는 겹치지 않습니다. 런타임 동작이 바뀌는 버그픽스라서, 문서만인 #3787 credits 수리 위에 올릴 가치가 큽니다. 다만 PR 본문이 말하듯 로컬 테스트/타입체크는 안 돌렸고 [skip ci]라서, 스택 꼭대기 Cross-platform CI가 사실상 검증 증거가 됩니다.

src/storage/cleanup.ts writePrivateFile - HEAD에서는 직접 writeFileSync라서 교체 중 크래시 시 매니페스트가 잘릴 수 있다. 이 PR이 그 경로를 고친다.
src/storage/cleanup.ts import - renameAtomicFile../config 대신 ../lib/windows-atomic-replace로 직접 가져온다. HEAD의 src/config/src/config/atomic-write.ts도 같은 구현을 재export하므로 동작은 같고, 스택 안 일관성만 보면 된다.
ExecuteCleanupOptions._test.beforeManifestReplace - 함수 훅이라 pickWireCleanupTestHooks에서 올바르게 제외됐다. wire로 새지 않는지만 최종 확인하면 된다.
tests/storage/storage-cleanup.test.ts - 예외 주입은 전원 차단 증명까지는 아니다. PR이 이미 적은 한계(디렉터리 fsync best-effort, 부분 영구 purge 비가역)를 머지 노트에 남기는 편이 좋다.
docs management-api.md / structure/02_config-and-codex-home.md - 복구 한계 문구가 코드와 맞다. 과장 없이 “이전 완전 기록 보존”과 “purge 되돌림 없음”을 같이 말한다.

메인테이너의 판단이 필요한 지점

너의 추천
드래프트 해제 후 Track 3 베이스로 머지 진행. #3778 실제 손상 경로를 막는 수정이고 HEAD의 satellite 원자 경로와 맞춘다. 로컬 스위트는 안 돌렸으니 스택 헤드 Cross-platform CI 통과를 머지 게이트로 두면 된다. types/config 분할로 무효화될 PR이 아니다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun
lidge-jun marked this pull request as ready for review September 6, 2026 20:19
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 6, 2026 20:19
@lidge-jun
lidge-jun merged commit 4397079 into dev Sep 6, 2026
13 of 14 checks passed
@lidge-jun
lidge-jun deleted the codex/track3-storage-manifests-a4b0 branch September 6, 2026 20:19
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-06T20:20:45.661530Z d682e06 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant