fix(update): gate the update on the proxy being down, not on stop's exit code - #3040
Draft
ntdatt812 wants to merge 1 commit into
Draft
fix(update): gate the update on the proxy being down, not on stop's exit code#3040ntdatt812 wants to merge 1 commit into
ntdatt812 wants to merge 1 commit into
Conversation
…xit code A dashboard update aborted after `ocx stop` had already stopped the proxy, because resume-history restoration exited non-zero and the update read the stop command's status as its gate. The port was left with no listener, the runtime state was cleared, and the old package was still installed. `ocx stop` does more than stop the proxy: it also restores Codex resume-history and reverts environment ownership, and reports failure when any of that fails. A history DB held busy by the Codex app is ordinary on Windows. The update's only real precondition is that nothing is still serving, since package replacement under a live proxy leaves it executing mixed old and new code. The gate now reads that condition alone and says what the stop reported. The resume-history warning below it was unreachable on exactly this path, which is the path it was written for. Refs lidge-jun#3008.
Contributor
|
Important Review skippedToo many files! This PR contains 1145 files, which is 845 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (20)
📒 Files selected for processing (1145)
You can disable this status message by setting the 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
Owner
리뷰 · 우선순위 69 / 80이 PR은 이슈 #3008을 고친다. 윈도우 대시보드에서 패키지를 올리려다 이 PR이 하는 일은 그 가드를 쪼개는 것이다. 형제 PR #3039 는 같은 복구 세션에서 나온 #3009 용이다. 그쪽은 라인 259 (src/update/index.ts, 현재 HEAD) - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3008.
A dashboard update aborted after
ocx stophad already stopped the proxy, because resume-history restoration exited non-zero. The reported end state was the bad one: nothing on the configured port, runtime state cleared, old package still installed.Why the status was the wrong gate
ocx stopdoes more than stop the proxy.handleStopalso reverts environment ownership and callsrestoreSharedClientStateAfterStop(), and any of those settingstopFailedproducesprocess.exitCode = 1. A Codex history DB held busy by the app is an ordinary Windows condition, so a clean proxy shutdown can still exit 1.The update read that status:
Its only real precondition is that nothing is still serving — package replacement under a live proxy leaves the running server dynamic-importing a mix of old and new modules, which is what the comment above that block already says. History restoration has no bearing on it.
There is a second symptom in the same lines: the resume-history warning immediately below the guard could not be reached on this path, because the condition it warns about had already aborted the update. It was written for exactly this case.
What changed
classifyStopForUpdatedecides both things and is exported for testing, next tohistoryRestoreIncomplete, which this file already exports for the same reason:The tray restore and
process.exit(1)on a genuine still-running proxy are unchanged.Tests
tests/update-stop-classification.test.ts, 4 cases:status: nullfromspawnSync) is not treated as a numeric failureI did not add a case to
tests/update-stop-first.test.ts; it drives real proxies, and the decision this changes is a pure one.Not in this PR
#3009 came out of the same recovery session, but it is
src/service.tsand a different failure point — the review on that issue asked to keep the two apart. It is up separately.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.