Skip to content

fix(test): settle fake checkpoint writer so main-slice test drains cleanly#25

Merged
onlyfeng merged 1 commit into
mainfrom
fix/main-ci-checkpoint
Jun 27, 2026
Merged

fix(test): settle fake checkpoint writer so main-slice test drains cleanly#25
onlyfeng merged 1 commit into
mainfrom
fix/main-ci-checkpoint

Conversation

@onlyfeng

@onlyfeng onlyfeng commented Jun 27, 2026

Copy link
Copy Markdown
Owner

Summary

checkpoint-main-slice test: the recording actor's spawn outcome never resolved,
leaving tryStartCheckpointWriter's settlement-watcher fiber suspended until scope
close force-interrupted it — a flaky source under it.live. Settle the outcome so
the watcher completes (writers.delete) and assert the writer drains to no-writer.

No production change — actor/spawn.ts is untouched; the test's spawn-ref finalizer
stays = undefined, consistent with the sibling checkpoint tests.

Relation to upstream

Same change set as XiaomiMiMo#1400 (fork → upstream), shared head branch.

Verification (local)

  • bun typecheck — exit 0
  • bun test test/session/checkpoint-main-slice.test.ts — 1 pass / 13 assertions

🤖 Generated with Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: efcf6bc83a

ℹ️ 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".

Comment thread packages/opencode/src/actor/spawn.ts Outdated
yield* Effect.addFinalizer(() =>
Effect.sync(() => {
if (spawnRef.current === impl) spawnRef.current = undefined
if (spawnRef.current === impl) spawnRef.current = previousSpawnRef

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid restoring disposed spawn refs

When two Actor.layer scopes overlap but do not shut down in strict LIFO order, this can resurrect a finalized service: scope A installs implA, scope B captures it and installs implB, A finalizes first and does nothing because current !== implA, then B finalizes and restores the now-disposed implA. Consumers such as checkpoint writer spawning read spawnRef.current later, and a stale actor service can attempt to forkIn A's closed scope. Consider tracking active entries/removing this impl from a stack rather than restoring a single saved pointer.

Useful? React with 👍 / 👎.

…eanly

The recording actor's spawn outcome never resolved, so tryStartCheckpointWriter's
settlement-watcher fiber stayed suspended and was force-interrupted at scope
close — a flaky source under it.live. Settle the outcome so the watcher runs to
completion (writers.delete) and assert the writer drains to "no-writer".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@onlyfeng onlyfeng force-pushed the fix/main-ci-checkpoint branch from efcf6bc to 946b80b Compare June 27, 2026 07:12
@onlyfeng onlyfeng changed the title fix(actor): restore previous spawn ref on layer cleanup fix(test): settle fake checkpoint writer so main-slice test drains cleanly Jun 27, 2026
@onlyfeng

Copy link
Copy Markdown
Owner Author

Reverted the spawn.ts spawn-ref change (and the test's matching save-restore). This PR is now the test-only flaky fix; the spawn-ref finalizer is unchanged from main and stays consistent with the sibling checkpoint tests' = undefined, which removes the restore-disposed-ref path the P2 comment flagged.

@onlyfeng

Copy link
Copy Markdown
Owner Author

CI note — the red unit shards are pre-existing base flakiness, unrelated to this PR.

  • Both failures are server live-runtime tests (workflows-route, session-prompt-heartbeat) returning HTTP 403 in CI; both pass locally (19 pass). 403 points at the CI environment (auth/network), not test logic.
  • The same two tests failed identically before and after the rollback (efcf6bc946b80b), confirming they're unrelated to the checkpoint change. The fork base 89f212b itself has a failing unit job on main.
  • Net change here is test-only (src/actor/spawn.ts untouched); all related tests pass locally.

@onlyfeng onlyfeng merged commit 6b88ae0 into main Jun 27, 2026
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant