Skip to content

fix(codex): serialize auto-redeem journal reservation and settlement - #3970

Merged
lidge-jun merged 3 commits into
devfrom
codex/248-b-journal-8456
Sep 8, 2026
Merged

fix(codex): serialize auto-redeem journal reservation and settlement#3970
lidge-jun merged 3 commits into
devfrom
codex/248-b-journal-8456

Conversation

@lidge-jun

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

Copy link
Copy Markdown
Owner

Summary

Serialize auto-redeem journal reservation and settlement across processes using the existing configuration mutation coordinator. Keep upstream consumption outside the lock, then re-read the durable reservation and verify its request ID before settlement so a delayed result cannot overwrite a peer's newer entry.

Carries #3912 from c817db47b28b44466036975577d4666083060d53 with original authorship. A scoped follow-up classifies both wrapped acquisition contention and raw SQLite commit contention for the existing short retry. Persistent storage errors retain the idle retry interval. The operation remains opt-in; no real account or credit is used for validation.

Verification

  • Independent source-plan audit approved the three-file boundary and real-database/cross-process regression design. Final independent source/security/test-design review PASS on a742211d0841989e67bdd5cc31ee7a35c08c9369; no blocking findings. Hosted execution pending.
  • Preserve original regression cases and add real COMMIT contention, two-process shared reservation identity with atomic barriers and bounded cleanup, and actual future-credit scheduling.
  • Current-head PR CI and ci.yml workflow_dispatch with lane=all are required. Skip/cancel/pending results are not passes.
  • Local product tests and suites NOT RUN per owner instruction. An earlier baseline-sync post-merge hook accidentally invoked install/typecheck/build; disclosed to the owner and excluded from verification. Subsequent Git mutations disable hooks per invocation and pushes use --no-verify.
  • This establishes local journal identity and coordination, not an externally verified upstream exactly-once guarantee.

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.

Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved reset-credit auto-redemption coordination when multiple processes share the same configuration directory.
    • Prevented conflicting redemption requests and preserved existing reservations during concurrent settlement.
    • Added retry handling for temporary storage lock contention and safer handling of failed or replaced reservations.
  • Documentation

    • Clarified that shared processes coordinate reset-credit redemption reservations and settlement.

luvs01 and others added 3 commits September 8, 2026 08:37
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 7, 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: 4517e278-c423-4d0f-9dbe-0f47f198f571

📥 Commits

Reviewing files that changed from the base of the PR and between bbea77a and a742211.

📒 Files selected for processing (3)
  • docs-site/src/content/docs/reference/configuration/server.md
  • src/codex/reset-credit-auto-redeem.ts
  • tests/codex-integration/codex-reset-credit-auto-redeem.test.ts

📝 Walkthrough

Walkthrough

The auto-redeemer now coordinates journal reservations and settlements with a shared SQLite-backed mutation lock. It retries lock failures, verifies reservation identity before settlement, and adds integration tests for process contention and journal integrity.

Changes

Reset-credit redemption coordination

Layer / File(s) Summary
Locked reservation and settlement flow
src/codex/reset-credit-auto-redeem.ts, docs-site/src/content/docs/reference/configuration/server.md
Reservations and settlements use withConfigMutationLockSync. Settlement re-reads the journal and rejects replaced request IDs. Lock failures use differentiated retry intervals. Journal retention uses the injected timestamp. The server configuration documentation describes cross-process coordination.
Reservation contention validation
tests/codex-integration/codex-reset-credit-auto-redeem.test.ts
Tests cover SQLite writer and reader contention, failed schema commits, retry reuse of request IDs, and two-process reservation races.
Settlement and edge-case validation
tests/codex-integration/codex-reset-credit-auto-redeem.test.ts
Tests cover delayed settlement, settlement retries, disabled ticks, injected timestamps, persistent journal write failures, and protection against replaced or missing reservations.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AutoRedeemer
  participant MutationLock
  participant Journal
  participant CreditService
  AutoRedeemer->>MutationLock: Reserve request ID
  MutationLock->>Journal: Read or write dispatched entry
  AutoRedeemer->>CreditService: Consume reset credit
  AutoRedeemer->>MutationLock: Settle request ID
  MutationLock->>Journal: Verify and write settled entry
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/248-b-journal-8456

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 7, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 7, 2026
@lidge-jun lidge-jun added the maintainer-sponsored Maintainer sponsors this change to an auth, workflow, release, or dependency surface label Sep 7, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

Explicit owner-authorized agent-assisted maintainer security review for a742211d0841989e67bdd5cc31ee7a35c08c9369 on bbea77a48adda1589d9dd298845255aeb96554aa.

Independent Astra high review and main verification accept the scoped journal change: reservation before network, network outside coordinator lock, settlement re-read and ID comparison, preserved opt-in and persistent-error backoff, short retry for wrapped/raw SQLite contention, and no account/credential-policy expansion. Real-database COMMIT-busy and two-process atomic-marker test designs were reviewed for reachability, distinct process identity, overlap, bounded retries and cleanup. All three approved files are accounted for; no shared coordinator change.

This is source/security acceptance and sponsorship, not a separate human approval or executed test claim. Exact-head hosted PR and lane=all CI remain gates. Fixtures are synthetic; no real reset credit or live account was used. No upstream exactly-once guarantee is asserted.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 62 / 80

이 PR은 Codex reset-credit 자동 환급(auto-redeem) 이 같은 OPENCODEX_HOME을 쓰는 여러 프로세스에서 저널을 동시에 고칠 때, 서로 다른 redeem_request_id를 만들어 크레딧을 두 번 쓸 수 있는 구멍을 막습니다. 지금 dev HEAD c46c22f3esrc/codex/reset-credit-auto-redeem.ts를 보면, 프로세스 안 inFlight만으로 tick을 직렬화하고, 저널 예약·정산은 잠금 없이 readJournal → 없으면 UUID 생성 → writeJournal → 네트워크 consume → 다시 writeJournal 순서입니다. 한 프로세스 안에서는 괜찮지만, 두 서버가 같은 설정 디렉터리를 보면 둘 다 “엔트리 없음”을 읽고 서로 다른 request id를 심은 뒤 각자 consume을 호출할 수 있습니다. 기여 원본은 #3912이고, 이 PR은 그 경계를 현재 dev에 올린 maintainer carry입니다.

고치는 방식은 이미 있는 설정 mutation coordinator(withConfigMutationLockSync / ConfigMutationLockError)를 저널 예약과 정산에만 거는 것입니다. 네트워크 consume은 잠금 밖에 두고, 응답이 온 뒤 잠금 안에서 저널을 다시 읽어 redeemRequestId가 예약과 같은지 확인한 다음 settled로 올립니다. 그래서 늦게 끝난 A가 B가 추가한 엔트리를 지운 채 옛 메모리 스냅샷을 덮어쓰지 않습니다. SQLITE_BUSY / locked는 1초 재시도, 그 밖의 저장 실패는 idle 재시도로 나눠서 바쁜 잠금만 짧게 돌게 했습니다. 문서 docs-site/.../server.md에도 “같은 설정 디렉터리를 공유하는 서버가 예약·정산을 맞춘다”는 문장이 추가됩니다. 테스트는 실제 SQLite COMMIT 경합, 두 프로세스 공유 예약 동일성, peer settled 보존, disabled tick이 coordinator 파일을 만들지 않는 경우까지 넣었습니다. #3965(수동 consume의 alias→canonical 장부)와는 같은 reset-credit 동네지만 경로가 다릅니다. 자동 환급 저널 vs 수동 operationId 정산이므로 둘 다 필요할 수 있습니다.

이 변경은 opt-in(resetCreditAutoRedeem)이라 기본 경로 사용자는 안 건드리지만, 기능을 켠 멀티 프로세스 환경에서는 크레딧 이중 소모를 직접 막아서 우선순위가 높습니다. types/config 분할에 걸려 rebase로 살릴 대상이 아니고, 원본 #3912는 이 carry가 들어가면 landed-via-maintainer로 닫는 패턴이 맞습니다. 본문도 로컬 product 테스트는 돌리지 않았고 hosted CI가 증거라고 밝힙니다. 업스트림 exactly-once를 새로 증명하는 게 아니라, 로컬 저널 신원·조율을 맞추는 범위라는 한계도 솔직합니다.

라인 src/codex/reset-credit-auto-redeem.ts · dispatch 예약 블록 - withConfigMutationLockSync 안에서만 UUID를 만들고 저널에 씁니다. 잠금 실패 시 retryJournal로 busy만 짧게 재시도하는 분기가 핵심입니다.

라인 src/codex/reset-credit-auto-redeem.ts · settle 블록 - consume 성공 후 저널을 다시 읽고 redeemRequestId가 바뀌었으면 throw합니다. 늦은 정산이 peer 엔트리를 지우지 않게 하는 안전장치입니다.

라인 tests/codex-integration/codex-reset-credit-auto-redeem.test.ts · peer settled 보존 / cross-process 테스트 - 두 harness가 같은 journalFile을 쓸 때 엔트리 두 개가 모두 settled로 남는지, request id가 각각 소비 id와 맞는지 확인합니다. 이 회귀가 없으면 옛 “덮어쓰기” 버그가 다시 열립니다.

경로 #3912 - 같은 수정의 contributor 원본입니다. 이 PR이 머지되면 Landed via #3970 at <sha> + landed-via-maintainer로 닫으세요.

경로 #3965 - 수동 reset-credit alias 정산 carry와 동시에 열려 있습니다. 충돌 파일은 거의 없지만, 머지 순서는 서로 독립으로 둬도 됩니다. 하나만으로 자동 환급 구멍이 닫히지는 않습니다.

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

  • hosted CI(lane=all 포함)가 이 head에서 초록인지, 스킵/취소를 성공으로 치지 않았는지
  • 정산 잠금 실패 시 upstream은 이미 응답했는데 outcome이 error로 남는 UX를 받아들일지(예약은 보존되어 같은 request id로 재시도)
  • fix(codex): serialize reset-credit auto-redeem journal updates #3912 leftover를 머지 직후 바로 닫을지

너의 추천
CI 그린이면 머지하세요. 멀티 프로세스에서 reset credit를 두 번 쓸 수 있는 실제 구멍을 기존 잠금 API로 막는 패치이고, 네트워크는 잠금 밖에 두어 잠금 시간도 짧습니다. 머지 후 #3912는 landed-via-maintainer로 정리하면 됩니다.

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

@lidge-jun
lidge-jun marked this pull request as ready for review September 8, 2026 00:19
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 8, 2026 00:19
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 8, 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-08T00:23:54.875158Z a742211 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.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration decision for a742211d0841989e67bdd5cc31ee7a35c08c9369 into dev e117b447fae2baf8ada26e982e41c12dcbcaefbc.

The owner authorized this independently revertible carry of #3912. Source/security review passed and was recorded above; the original author survives in the source commit and final squash trailer. Exact-head PR CI 34170889491 succeeded; workflow_dispatch 34170970202 succeeded with all 26 jobs, including all six Windows shards and macOS control. Windows execution logs confirm the new COMMIT-contention, two-process ID-sharing, disabled-boundary and future-credit cases passed. No local product execution is claimed beyond the disclosed excluded baseline-hook incident.

The destination moved by C delivery-record documents plus the independent Santiago test isolation in gui/tests/usage-custom-range.test.tsx. Independent integration review found no journal dependency impact. Journal/runtime tests, dependency pins, workflow and docs are identical to candidate inputs. We reuse these results as journal-scoped evidence; we do not claim the whole combined tree was executed. Its only differences from the candidate must be that GUI test and C delivery records, and actual landing will be compared to the precomputed merge tree.

This is explicit owner-authorized dev-only maintainer integration under MAINTAINERS.md, not a separate human approval. No maintainer objection or unresolved review thread is present. Ready-triggered review automation may still be pending and is not reported as passed. The shared merge lock covers final revalidation through landing proof.

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

Labels

bug Something isn't working maintainer-sponsored Maintainer sponsors this change to an auth, workflow, release, or dependency surface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants