Skip to content

fix: tighten openapi copy folding and the sqlite interrupt test - #60794

Merged
kitlangton merged 2 commits into
devfrom
effect-41-followups
Oct 31, 2026
Merged

kitlangton merged 2 commits into
devfrom
effect-41-followups

Conversation

@kitlangton

@kitlangton kitlangton commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Three small fixes from the review of the Effect 4.1 upgrade, plus a simplify pass on openapi-stabilize.ts. openapi.json and the generated clients don't change.

❌ Before

  1. A chain of schema copies (X, X_2, X_2_1) was folded only one step, leaving a reference to a deleted schema.
  2. An unsuffixed first *Encoded schema kept its encounter-order name, so reordering events could change its meaning.
  3. The sqlite interrupt test released the connection before the queued fiber waited, so it never tested the waiter queue.

✅ After

  1. References to a copy resolve through the whole chain to the original.
  2. <Base>Encoded is renamed by content whenever a <Base>Encoded_N sibling exists.
  3. The holder yields 22 times and the sweep covers 45 delays, so the queued fiber really parks (confirmed with a temporary probe on the semaphore).
- resolve(X_2_1) = X_2        // dangling once X_2 is removed
+ resolve(X_2_1) = X          // followed to the original

The stabilize script also got smaller (151 → 102 lines): one ref walker and one canonical form now serve both content addressing and copy folding, and copy equality compares canonical shapes with references resolved through the copies still folding. That replaced the custom deep-equal, the one-step reference matcher and the separate chain-flattening loop.

Case Without fix With fix
3-level copy chain test ❌ ✅
Unsuffixed Encoded test ❌ ✅
Interrupt test, old take(1) acquirer ❌ leak at delay 24 (3 of 4 cases) ✅

New stabilize tests fail without the fix; the interrupt test passes on dev in about 90 ms.

👀 Read: openapi-stabilize.ts, the anonymous filter and collapseCopies. The rest is routine.

Follow-ups from the review of #60651 · no user reports

Checks run
  • bun test test/openapi-stabilize.test.ts (protocol): 9 pass; mutating chain resolution, the Encoded rule, the family rule or the fixed-point loop each fails a test
  • bun test test/sqlite-interrupt.test.ts (core): 4 pass in 90 ms; 3 fail with the old take(1) acquirer
  • bun run generate, check:generated (protocol, client): no diff
  • bun run typecheck (protocol, core): clean

@opencodejr opencodejr 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.

The fix looks good to me. On the base, the two new stabilize tests fail; with this PR they pass. check:generated shows no diff, the sqlite interrupt test passed 8 times in a row, and typecheck:tsc is clean in protocol and core.

The flattening step runs after the equality pass, so a copy whose parent stays unfolded still points to a schema that exists. Matching the unsuffixed <Base>Encoded only applies when a suffixed sibling exists, so authored names are left alone.

Public API: unchanged. The change touches only the spec-generation script and tests, and the generated openapi.json and clients stay the same.

One renameRefs walker and one canonical (sorted, optional inlining) serve both content addressing and copy folding. Copy equality compares canonical shapes with references resolved through the remaining copies, which also replaces the separate chain-flattening step. openapi.json is unchanged.

@opencodejr opencodejr 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.

The new commits look good. The refactor keeps the earlier behaviour: the stabilize tests pass (9), check:generated shows no diff in protocol and client, and typecheck:tsc is clean. Resolving references through the whole copy chain covers the dangling-reference case without the separate flattening loop. No new concerns. Public API: unchanged; this only touches the spec-generation script and tests, and the generated output is the same.

@kitlangton
kitlangton merged commit 8cfea10 into dev Oct 31, 2026
36 of 37 checks passed
@kitlangton
kitlangton deleted the effect-41-followups branch October 31, 2026 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant