Skip to content

[RT-E2] refactor: terminals run on the client's TimeProvider - #432

Merged
Blind-Striker merged 2 commits into
masterfrom
feat/rt-e2-b-term
Oct 17, 2026
Merged

Blind-Striker merged 2 commits into
masterfrom
feat/rt-e2-b-term

Conversation

@Blind-Striker

@Blind-Striker Blind-Striker commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

What

The PTY and persistent-PTY terminal families now run their send deadlines and graceful-close bounds on the client's TimeProvider. This is RT-E2 PR B of three. A (#429, background service and TimedCancellation) has merged; C (the launcher and the clock ban) follows.

Closes

  • U2-08: ITerminalDeadlineFactory duplicated TimeProvider, and the close bounds bypassed it. ITerminalDeadlineFactory and TerminalDeadlineFactory are deleted.
  • U2-31: neither graceful-close bound had a test, and both ran on real timers. Both bounds are now on the client clock, with new tests.
  • U2-16: TerminalCancellationSource only forwarded to a CancellationTokenSource. It is deleted; raw sources with CancelOnWorkerAsync replace it.
  • U2-34: the CancelOnWorkerAsync contract had no test. It has one now.

The change

  • No terminal composition root. ConnectionSnapshot carries the client's clock from Pipeline. PtyClient.ConnectAsync and the persistent attach pass it to the sessions, then TerminalSocketCore, TerminalSendQueue and each send deadline, as required internal parameters with no System fallback.
  • The per-send deadline is still armed before payload preparation and admission.
  • The two graceful-close bounds stay independent: the admission bound first, then a fresh close-output bound after acquisition. There is no shared budget and no leaked semaphore waiter.
  • All three deadline sites use TimedCancellation.Create from [RT-E2] refactor: the background service runs on an injected TimeProvider #429, so the downlevel timer is released on an early dispose.
  • Compile-forced edits only: an explicit clock is supplied, and no assertion or test name changed. The files are the terminal, PTY and persistent-PTY tests, ConnectionSnapshotTests, CancellationIdentityTests, and three benchmarks, which pass TimeProvider.System; the ban in PR C covers src only. U2-07's real-timer races belong to TST-E2b and are not rewritten here.

Proof

  • Real-time sites in the terminal slice: 3 went to 0, with no terminal root.
  • New seam proofs on ManualTimeProvider. Each is killed by a compiling product mutant that keeps the signature, on net10.0 and net472:
    • TerminalSendQueueTests: the admission bound, the close-output bound, and the two-budget case;
    • CancellationTokenSourceExtensionsTests: CancelOnWorkerAsync returns while a callback blocks, and completes only after it.
  • Two test fixes made during review:
    • A host-killing test (Sol, should-fix). TerminalSocketCoreTests' queued-expiry test threw a TimeoutException inside an AsyncLocal change callback. An exception there triggers Environment.FailFast and takes the whole test host down. The callback no longer throws: it records the wait, and the test asserts it on the test thread. A never-expire mutant now fails the test normally.
    • A racy hold (found by the local check). That same test held the first send through the AsyncLocal callback. Under full-suite scheduling the callback could fire on an unrelated continuation carrying the same execution context, so the first call released the gate early and the test timed out. The hold now runs in the core's own post-send step (TerminalSendActions.OnSent), on the first call's path, after the physical send and before the gate is released. It is deterministic.
  • Clone baseline. tools/jscpd/tests.baseline.json makes 24 one-for-one swaps of pre-existing clones that the compile-forced clock argument re-fingerprinted, plus one removal of a clone this branch eliminated, so it goes from 655 to 654 entries. The 2 clones the branch introduced were refactored into helpers (CreateGatedCloseQueue, AssertAdmissionReleasedAsync). Consolidating the 24 pre-existing arranges is TST-E4's work, not a product PR's.
    • Mapping, old → new: 410dc16d23b0fb80→c2a7b415518d2198, b1f194514a39741f→ba50dc522914af62, bb9c534e42d8fbec→ceed549c863ce378, 80cfe1c4da15558e→7cb3773f240a7d3b, 614c8cb7e327b799→ba1b81c7dacd299e, eb7e225d5bfe0159→7baca2d0e7c78a56, 9111880caf330ba5→2fcfde38c702a305, d9ac5bae878d22b3→63ccee6cade2a08c, b107b4bdff30ec7a→3081885a80a22be3, cbf3676e991f1146→0fcc7cca1b6402dd, 85079fc139806e83→4202e901bcb4f8ba, 70bd27545e44e77c→f37319ef3f0e2c88, dee72b571774c9d5→210bbefc7e3a3805, ca8b61da9bd303bb→58df2dce6a57ddc3, 2ad053ce77d6dfb2→a5830a7e1b5d7975, 064bde5a422105ed→13fa62aa808a28cb, 3bccb11c8653a298→a644bdfbe6ec0044, a7b53cc19a267d21→50c75dddf97e272d, 0d1c1e62f027669f→93c7ffbef646d5bd, e45d1d2fb94c859b→813356530d4e26c5, c3312d7a632e3665→ee67ec3f21f2f9ce, 33df1e6ccf3c7b58→030445ef25b69be7, ceefd01b00d7a785→8845f6d375146134, 9c41941b90bc5dc4→2d01f1bea1f9ea7e; removed a86ca42b60a2d816
  • Registry: ReservedNamePolicy.InternalTypeNames drops the three deleted types.
  • Checks: build 0/0 on every TFM; both format passes; slopwatch 0; verify-clones 0 new (tests 5.60%, against the 6.0% ceiling); the whole OpenCode.Sdk.Tools.Tests project 2008/2008; Terminal, Ptys, PersistentPtys and Threading green on net10.0 and net472. PublicApi.verified.txt is unchanged.
  • Local check: GATE PASS on 315ed80f (this branch on master 62d82f0a): slopwatch, build, both format passes, clone-detector, verify-clones, 10,457 tests including net472 with 0 failed, and election. An earlier run caught the racy hold described above. The second commit, which only renames two rows and adds a registration, was rerun narrowly: TimedCancellationTests 4/4 on net10.0 and 5/5 on net472, the whole OpenCode.Sdk.Tools.Tests project 2005/2005, slopwatch, format and clones.
  • Reviews:
    • The Runtime coordinator.
    • A first fresh Sol review: one should-fix, the FailFast above, now fixed.
    • A Sol delta review after the rebase and the factory switch: no findings.
    • A Grok concurrency review of TimedCancellation and this switch, which the maintainer approved for lock and race code: "the design is sound", no blocker or should-fix. Two nits are recorded, not fixed:
      • a second Dispose would skip base.Dispose only if _timer.Dispose() threw, which BCL Timer.Dispose does not;
      • two TimedCancellationTests rows claimed more than they proved. This one is fixed in this PR's second commit: they are renamed to a callback queued after Dispose and a race carrying a registration, so the callback's Cancel runs work under the lock while Dispose contends.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

ITerminalDeadlineFactory, TerminalDeadlineFactory and TerminalCancellationSource
are gone. ConnectionSnapshot carries the client's clock; PTY and persistent PTY
sessions pass it to the terminal core, its send deadlines and both graceful
close bounds, which stay independent. New manual-clock tests prove the
admission bound, the close-output bound, the two-budget case and the
CancelOnWorkerAsync contract.
The disposed-deadline row fires a callback queued after Dispose, not one that
overlaps it, and the barrier race now carries a registration so the callback's
Cancel runs work under the lock while Dispose contends for it.
@Blind-Striker
Blind-Striker added this pull request to the merge queue Oct 17, 2026
Merged via the queue into master with commit b4feb7b Oct 17, 2026
12 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