Skip to content

fix(server): self-heal a replaced package tree via drain-and-restart - #5513

Closed
luvs01 wants to merge 16 commits into
stack/c-svc-startupfrom
stack/c-svc-selfheal
Closed

luvs01 wants to merge 16 commits into
stack/c-svc-startupfrom
stack/c-svc-selfheal

Conversation

@luvs01

@luvs01 luvs01 commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Self-heal a replaced package tree via drain-and-restart, combining #5393 with its follow-up scheduler fix in the second layer of the service stack.

  • fix(server): self-heal a replaced package tree via drain-and-restart #5393: detect a replaced installed package tree, degrade health honestly, and drive a drain-and-restart instead of serving a mismatched runtime.
  • Follow-up (CodeRabbit finding): defer the verify step via queueMicrotask so a synchronous scheduler cannot re-enter armRestartTimer before cancelScheduled ownership settles.
  • Source commits cherry-picked with -x provenance. Source migration evidence is recorded below.

Verification

  • Current head: 4d168f16654eefc04878f98156d40b0f5eedd964. This follow-up changes only documentation and the specific test consumers listed below; production source is byte-identical to the preceding published head c2434867ec5e7d50604442a395cee1f7d4724c04.
  • Hosted CI exposed structure/runtime.md growing to 602 lines when the branch was combined with newer dev. Two overlapping Remote Workspace descriptions were consolidated without losing their contracts or links. The same patch was verified against both exact failing merge trees (602 to 600) and dev (600 to 598), without raising the 600-line budget. Final combined structure checks passed.
  • The final service-fixture behavior was tested at b837cc09fec5e04a1165f4f0d07b4649fd0c914d: pinned Bun 1.4.0, three explicit files (package-tree-integrity, server-auth-localhost-bind, server-auth) filtered to package integrity / dotted localhost / management CORS / non-loopback management: 28 passed, 0 failed, 110 filtered, 119 assertions, 16.92 seconds. Those runtime and fixture files remain unchanged by this documentation follow-up. This is not the entire auth suite.
  • Earlier samples failed with 26 passed / 2 hook timeouts, then an instrumented 27 / 1. Instrumentation localized redundant startup migrations and deferred real ACL work inside beforeEach. Current-schema fixture projection removes those unrelated upgrade writes before one real saveConfig; actual ACL, token, lease, listener, guards, assertions and 5-second/900-second limits remain. Typecheck, structure and size checks passed after that correction; the final lower-layer typecheck and final upper-layer structure check also passed.
  • Full-suite/cross-platform completion, exact updated-head hosted CI and independent security review remain incomplete. No test timeout or safety boundary was weakened. This PR stays draft.

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.

Review readiness

  • Local CI is green on this exact head.
  • Branch is based on the current dev commit at preparation time.
  • All correct Codex and CodeRabbit findings are fixed on the combined head.
  • Ready-for-review confirmation.

An out-of-band package-manager upgrade (npm/bun/pnpm global install) under a live proxy replaced the package manifest and fenced every /v1/* request behind 503 package_tree_changed until a manual restart. The integrity guard now accepts an onReplaced callback: after a replaced observation persists past a short debounce (and resets if the manifest goes unreadable mid-install or recovers), the server accepts the existing graceful drain-and-restart path, so the new tree comes up on its own. Source checkouts and standalone binaries still never fence, so development and single-file installs are unaffected.

(cherry picked from commit 3f33e4f)
Drive recovery from an unref'd stability timer so a replaced package tree restarts without a second request. Require the same readable replacement identity for the full debounce, retry restart admission failures, cover server wiring and baseline recovery, and document the lifecycle contract.

(cherry picked from commit 4eac35c)
A synchronous schedule seam could invoke the restart callback before cancelScheduled was assigned, letting verifyAndNotify re-enter armRestartTimer and orphan a stale timer. Queue the verify step as a microtask so ownership settles first; scheduler-driven tests now drain one microtask per runNext, with new coverage for a synchronous schedule, a throwing onReplaced, and disposal before the queued verify.

(cherry picked from commit c66e0b368de36ab7e38ecffacebd4bb2560a5638)
@luvs01
luvs01 added this pull request to stack #5514 September 22, 2026 09:47
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions

github-actions Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed.

Hygiene

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 22, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 61 / 80

이 PR은 설치된 npm/bun/pnpm 패키지에서, 서버가 돌아가는 동안 패키지 매니저가 트리를 통째로 바꿔 버렸을 때의 문제를 고칩니다. 예전에는 package.json 정체성이 바뀌면 /healthz·/readyz·/v1/*가 package_tree_changed 503으로 막힌 채, 사람이 직접 재시작할 때까지 그대로였습니다. 이제는 같은 새 정체성이 약 5초 동안 읽기 가능한 채로 유지되면, 요청을 한 번 더 기다리지 않고 기존 drain-and-restart 경로(acceptSystemRestart)로 새 트리를 다시 올립니다. 설치 중간에 매니페스트가 잠깐 안 읽히면 타이머를 리셋하고, 예전 정체성으로 돌아오면 재시작을 취소하며, server.stop()에서는 dispose()로 대기 중인 재시작을 끕니다. 소스 체크아웃과 단독 바이너리는 예전처럼 이 울타리를 쓰지 않습니다. 가드·타이머·서버 배선 테스트와 structure/runtime.md 설명도 같이 들어왔습니다.

라인 tests/ci-workflows/file-size-ratchet.test.ts - CI test 4/4가 여기서 깨짐. src/server/index.ts가 903줄로 GREW 판정. 재시작 배선·import가 index에 더 붙어서 크기 래칫을 넘김.
라인 PR body - enforce-target이 bad description (empty)로 실패. Summary/Verification이 템플릿 그대로라 품질 게이트가 DRAFT로 유지됨.
라인 src/lib/package-tree-integrity.ts armRestartTimer - replacedRestartDelayMs === 0일 때는 queueMicrotask 없이 verifyAndNotify를 바로 호출함. 동기 schedule 재진입을 막으려고 넣은 지연이 이 경로에는 없음(테스트 시나리오·0ms 설정일 때만 해당).

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

base가 dev가 아니라 stack/c-svc-startup인 스택 PR이다. 게이트도 stacked로 보고 wrong_base는 건너뛰었다. 이 셀프힐을 스택 중간에서 합칠지, dev로 다시 겨냥할지는 스택 순서에 달려 있다. 또한 index.ts 성장을 허용(래칫 갱신)할지, 배선을 index 밖으로 빼서 GREW를 없앨지 정해야 한다.

너의 추천

방향은 좋다. 머지 전에 (1) PR 본문에 Summary/Verification을 실제 내용으로 채우고, (2) src/server/index.ts GREW를 해소하거나 래칫을 의도적으로 갱신한 뒤 CI를 다시 통과시켜라. 스택이 맞다면 base는 유지해도 된다.

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

@luvs01 luvs01 changed the title stack/c svc selfheal fix(server): self-heal a replaced package tree via drain-and-restart Sep 22, 2026
Move the default guard wiring into src/server/index/package-tree-guard.ts so index.ts stays under the file-size ratchet cap; behavior and the injected-deps contract are unchanged.
The merge result pushed runtime.md over the 600-line structure budget.
Keep a pointer in runtime.md and carry the full contract in
ops/docs-and-release.md under the package runtime section.
… scheduled path

The synchronous delayMs === 0 branch ran verifyAndNotify inside the arming frame, so a zero-delay verify could re-enter armRestartTimer while that arm was still running. Defer it through a microtask so the zero-delay path follows the same re-entry rule as the scheduled one, and pin the contract: the arming status() returns before the verify, a callback re-entering status() still fires exactly once, and a dispose or baseline recovery before the flush means the verify never fires.
lidge-jun added a commit that referenced this pull request Sep 22, 2026
…icit shutdown

Security review follow-up to the #5513 carry. Once an automatic package-tree
restart entered its drain, an operator shutdown (signal or management stop)
could still be followed by the restart handoff, because the drain cannot tell
its own listener stop from an independent one. Explicit shutdown paths now mark
the process, and an admission-bound restart checks that mark before every
handoff step. Manually requested restarts keep their behavior.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Carried into #5610 as 6c2a0e2 (squash of this PR's own diff at head 4d168f1, authorship kept). Review follow-ups in #5610 (48580c4, f5276bf, bb37df7) veto an accepted package-tree restart when the server stops before it begins. They also recheck service-home ownership at admission and before handoff, and block the handoff after an explicit operator stop. Closing as superseded by #5610. Thanks @luvs01.

@lidge-jun lidge-jun closed this Sep 22, 2026
lidge-jun added a commit that referenced this pull request Sep 23, 2026
)

* fix(service): combine startup ownership, token binding, and slot retention

Carries #5512 by @luvs01 (head a12b2ad), which
consolidates #5477, #5306 and #5357:

- bind the service API token to its owning state, canonicalize qualified-localhost
  binds, and carry WSL ownership state honestly (#5477);
- take a fresh task listing for the second startup ownership decision (#5306);
- retain workflow slots for streaming turns (#5357);
- own server-auth fixture lifetime and project a current-schema config for it.

Squashed from the PR's own diff (origin/dev...a12b2ad) onto current dev.

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

* fix(server): self-heal a replaced package tree via drain-and-restart

Carries #5513 by @luvs01 (head 4d168f1), which
consolidates #5393 and its scheduler follow-up: detect a replaced installed package
tree, degrade health honestly, and drive a timer-driven, retryable drain-and-restart
whose verify step is deferred past scheduler re-entry. The guard factory lives in
src/server/index/package-tree-guard.ts.

Squashed from the PR's own diff (a12b2ad...4d168f1) onto the #5512 carry.

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

* fix(security): combine install discovery, credential, and transport hardening

Carries #5515 by @luvs01 (head 843f299), which
consolidates #5359, #5285 and #5322:

- keep selected Codex installation discovery off network filesystems, probe
  oversized wrappers through a held-handle prefix read, and stop a PATH scan at a
  refused probe (#5359);
- exclude npm candidates inside the launch directory subtree (#5285);
- refuse plaintext remote hub origins, fail closed on POSIX chmod for credential
  files, and skip the frame-log write when descriptor hardening fails (#5322).

Squashed from the PR's own diff (origin/dev...843f299) onto the chain carry.
Integration: structure/runtime.md wording reflowed by two lines so the combined
service and security stacks stay within the 600-line structure budget.

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

* fix(security): combine management-auth and boundary hardening

Carries #5516 by @luvs01 (head 245d542), which
consolidates #5326, #5312, #5363 and #5317:

- harden pairing redemption, agent roster intake, and SOCKS5 decoding (#5326);
- guard gh resolution, anchor the grok managed-region fences to whole lines, and
  bound provider-controlled text (#5312);
- harden management-auth admission and provenance (#5363);
- bound the /healthz version before it reaches diagnostics (#5317).

Squashed from the PR's own diff (843f299...245d542) onto the #5515 carry.
Integration: both stacks rewrote the shared server-auth test fixtures. The carry
keeps the #5512 current-schema fixture projection and config helper (including
its 4 KiB boundary case) and adds this PR's Aside sync capability assertions.

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

* fix(security): combine adapter argv and upstream-body hardening

Carries #5517 by @luvs01 (head 260a87b), which
consolidates #5315 and #5336:

- stage Qoder and CodeBuddy system prompts in private files instead of
  child-process argv, with exclusive creation and owned cleanup (#5315);
- bound upstream error bodies and resolve account-scoped transports (Copilot,
  Devin) from the same OAuth snapshot as the bearer (#5336).

Squashed from the PR's own diff (245d542...260a87b) onto the #5516 carry.

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

* feat(codebuddy): integrate capture-only tools with private prompt staging

Carries #5582 by @luvs01 (head 3061ef9), which
integrates the capture-only CodeBuddy tool bridge from #5148 by @mdwsk88 with the
private prompt staging from #5517. Requests with a tool catalog advertise only the
allowed tools through an isolated MCP server that captures calls without executing
them; the client keeps approval, sandboxing and execution. Pre-init, undeclared,
excessive or incomplete calls are rejected, streamed malformed tool arguments are
suppressed, bridge staging failures return a fixed message, and an opt-in live
acceptance harness is included. Design context: #5146.

Squashed from the PR's own diff (260a87b...3061ef9) onto the #5517 carry.

Co-authored-by: mdwsk88 <924038395@qq.com>

* fix(client): bound total hub catalog response lifetime

Carries #5252 by @luvs01 (head 779ef91): give the
hub catalog body read an overall deadline (24x the inactivity window, capped at
120 s) on top of the inactivity window, and release refused, HTTP-error and 304
bodies without awaiting their cancellation.

Squashed from the PR's own diff (origin/dev...779ef91).

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

* fix(grok): reserve model aliases only when the written config stays valid

Reimplements #5281 by @luvs01. A user sub-table such as [model.ocx-mine.extra]
only creates an implicit parent, so it no longer forces the generated table to a
suffixed alias. The alias choice is now checked against the bytes actually
written: the unsuffixed alias is used only when the final config (after
model-reference rewriting) parses; otherwise the conservative choice that also
reserves deeper headers is used, and a valid user file for which neither choice
parses is refused without writing. Malformed user TOML keeps the previous
conservative reservation.

The original change reserved only exact two-segment headers, which could emit a
duplicate [model.x] table when the user defines model.x through dotted keys.

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

* fix(codex-auth): scope Codex OAuth cancellation to the originating flow

Reimplements #4923 by @luvs01 on the current login-state layout (in-flight
controllers moved to src/oauth/login-flow-state.ts in #5220). Cancelling a Codex
login was keyed only by provider, so a stale modal posting an old flowId could
abort a newer attempt, and a cancel without a flowId expired every pending flow.

- Each in-flight controller records the flowId that started it; a cancel whose
  flowId does not match the active attempt is refused before anything aborts.
- POST /api/codex-auth/login/cancel requires a non-empty flowId, rejects unknown
  or non-pending flows with 400 without touching any row, and expires only that
  flow. Provider-wide cancellation through /api/oauth/login/cancel is unchanged.
- ocx account cancel requires --flow for Codex providers and sends no request
  without it.

The dashboard's 409 recovery keeps its code; its ownerless cancel is now refused,
so it ends in the existing "already in progress" message instead of superseding a
flow it does not own.

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

* fix(socks5): bound compressed event streams by expansion, not total size

Review follow-up to the #5516 carry. The 32 MiB decoded-body cap applied to every
gzip/deflate response, so a long, normally compressed SSE stream through the
SOCKS5 tunnel was cut once its cumulative output crossed the cap. Buffered
responses keep the absolute cap; event streams may continue while decoded bytes
stay within the greater of 32 MiB or 128x the coded bytes consumed, which still
stops high-ratio bombs.

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

* fix(codex): keep scanning PATH past a missing Windows candidate

Review follow-up to the #5515 carry. The held-handle reader reported a missing
file or directory as open-refused, so the default existence probe stopped the
PATH scan at the first absent PATHEXT candidate (for example codex.com) before it
reached an installed codex.cmd. NtCreateFile's object-name-not-found and
object-path-not-found statuses now map to a distinct not-found result that lets
the scan continue; every other failure still refuses.

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

* fix(server): require Windows ACL hardening before a frame-log append

Review follow-up to the #5515 carry. On Windows the frame log ignored a failed
permission change and appended anyway. Each append now hardens the target with
the required Windows ACL helper and checks that the path still names the opened
file before writing; any failure writes nothing.

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

* fix(devin): bind catalog authority to the tenant destination

Review follow-up to the #5517 carry.

- The observe-only OAuth snapshot applied the Copilot-validated apiBaseUrl to
  every provider, so a crafted Devin credential could carry a Copilot host that
  the snapshot claimed as its own. The overlay now applies only to github-copilot.
- Devin's live roster, stale fallback and cooldown were keyed by the token alone
  while discovery also depends on the validated tenant URL. The catalog authority
  and the matching routing-cache resolver now fingerprint the token together with
  the validated destination URL.

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

* fix(codebuddy): fail closed on unverified bridge turns and staging collisions

Review follow-up to the #5582 carry.

- With the capture-only tool bridge armed, a successful terminal event is no
  longer accepted unless the CLI's system/init frame confirmed the bridge server;
  a turn that ends without it fails with tool_bridge_init_missing.
- A tool_use block that arrives only in the complete assistant message, without
  the partial tool events the bridge captures, now fails the turn instead of
  being dropped silently; partial captures are deduplicated by id.
- The catalog and MCP config staging files are created exclusively (wx, 0600),
  like the prompt file, so a pre-existing file fails before spawn.
- The history-argument repair for a missing JSON object prefix is documented and
  tested as a provider-agnostic contract; other malformed strings keep {}.

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Co-authored-by: mdwsk88 <924038395@qq.com>

* fix(service): keep service-command ownership bound to the recorded home

Review follow-up to the #5512 carry. On WSL with CODEX_HOME unset, the carried
allowance treated a legacy Linux ~/.codex install record as owned when discovery
now selects the Windows profile, so service stop could stop the Linux-home
service and then restore native Codex in the Windows home, and repair could
rewrite the recorded home. Service commands again require the exact recorded
home and name it in the refusal; the unattended startup inspector reaches the
same foreign verdict.

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

* fix(server): veto a package-tree restart when its server stops or loses ownership

Review follow-up to the #5513 carry.

- A package-tree restart accepted by the guard stayed scheduled after an explicit
  server.stop(), so the drain-and-respawn could reopen a server the caller had
  stopped. The caller that accepted a pending restart now receives a veto, and
  the guard uses it on dispose.
- When running as a supervised service child, the automatic path checks service
  home ownership when accepting and again before the handoff; a mismatch keeps
  the 503 fence and skips the restart.

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

* fix(security): resolve gh from fixed paths and look up pairing grants by digest

Review follow-ups to the #5516 carry.

- On Windows the automatically polled star-status route derived gh.exe roots from
  ProgramFiles and LOCALAPPDATA, so a process environment could select any
  absolute directory. Windows candidates are now the fixed system install paths,
  and the child PATH is only the resolved executable's directory. Other installs
  report gh as unavailable, which only hides the sidebar star state.
- Pairing redemption looked each guess up by scanning every live grant; the map
  is keyed by the grant digest, so the lookup is now a direct get. A valid grant
  still redeems behind a throttled source.

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

* test(server): cover the one-shot Aside sync capability end to end

Review follow-up to the #5516 carry, which added a one-shot, HMAC-bound
capability for the default ocx sync path without exercising it. A real listener
now proves single use, refusal on replay, wrong path, query, method, pid or port,
expiry and a bad MAC, and that the CLI default path performs the attestation and
a bodyless POST (through a narrow transport seam).

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

* test: register the review follow-up test files in the layout maps

Adds the three new test files from the L4 review follow-ups to both
scripts/test-layout/layout.json and tests/fixtures/test-layout-expected.json.

* test(grok): pin re-injection and strip for a nested user model table

Review follow-up to the #5281 reimplementation: two injections are byte
identical, every intermediate file parses, and strip restores the exact user
content.

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

* fix(server): harden a Windows frame log once per file identity

Re-review follow-up: requiring Windows ACL hardening on every append spawned
icacls for every relayed frame and could stall the realtime relay. The hardened
file identity (device and inode) is now remembered for the log path; an
unchanged file skips the respawn, and a replaced file at the same path is
hardened again before any write.

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

* docs(structure): describe the package-tree restart veto and ownership recheck

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

* fix(server): stop an automatic restart from handing off after an explicit shutdown

Security review follow-up to the #5513 carry. Once an automatic package-tree
restart entered its drain, an operator shutdown (signal or management stop)
could still be followed by the restart handoff, because the drain cannot tell
its own listener stop from an independent one. Explicit shutdown paths now mark
the process, and an admission-bound restart checks that mark before every
handoff step. Manually requested restarts keep their behavior.

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

* fix(server): mark a management stop before its asynchronous teardown

Security re-review follow-up: the management stop route marked the explicit
shutdown only after awaiting the shared teardown, so an automatic restart
draining concurrently could reach its handoff in that window. The mark now
precedes the first await after the stop is accepted.

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

* test(server): allow post-lookup pruning in the pairing digest regression

The digest-lookup regression trapped every iteration of the grant map, so a
valid redemption failed once session minting pruned expired grants after the
lookup (hosted CI test 4/4). The trap now fails only on a scan that precedes the
digest lookup, which is the regression it guards.

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

* fix: repair standalone bridge and restart ownership

Use the compiled CLI as the capture-only MCP entrypoint, release automatic restart fences on veto, align Devin discovery, and tighten Windows and local transport handling. Apply the documented Qoder prompt environment for both regions and update focused regressions and operator docs.

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

Co-authored-by: mdwsk88 <924038395@qq.com>

---------

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Co-authored-by: mdwsk88 <924038395@qq.com>
@lidge-jun
lidge-jun deleted the stack/c-svc-selfheal branch September 26, 2026 01:54
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.

2 participants