Skip to content

feat(system): expose shared terminals locally - #71

Merged
nicodes merged 7 commits into
mainfrom
feature/504-local-terminal-client
Aug 24, 2026
Merged

feat(system): expose shared terminals locally#71
nicodes merged 7 commits into
mainfrom
feature/504-local-terminal-client

Conversation

@nicodes

@nicodes nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Closes nicodes/ormos-be#504

Risk: high. This changes shared PTY input/output, connection limits, local directory-policy enforcement, and multi-client concurrency.

Corrections

  • The worker rechecks POLLOUT under the shared input lock and writes at most 512 bytes, the portable Linux/Darwin PIPE_BUF floor, so the write cannot block. Correction: PIPE_BUF applies only to pipes/FIFOs, not PTY masters, and POSIX does not make a subsequent blocking write safe after poll. The current head instead normalizes the PTY master to permanent O_NONBLOCK and routes all browser/local input through one session scheduler whose sole kernel write is raw nonblocking unix.Write.

  • The shared session scheduler is bounded at exactly 256 calls and 1 MiB. Correction: that bound let a legal maximal local paste disconnect the next browser keystroke. Local aggregate admission remains exactly 256 calls/1 MiB; the scheduler now adds a bounded 64-call/256 KiB browser reserve, for exact combined bounds of 320 calls/1.25 MiB. Browser readers wait on capacity release rather than disconnecting.

  • The 256-call/1 MiB session-local bound was reserved before input copying. Correction: on 38b55d6, only each attachment reserved before copying; competing attachments could allocate beyond the session aggregate before rejection. The current head reserves per-attachment, session-local, and total scheduler accounting in one ordered transaction before any copy, then rolls all ledgers back exactly once if publication fails.

Summary

  • add the shared terminal-session control-plane DTO with pinned snake_case wire tags
  • generalize terminal fan-out so sealed browser and bounded local clients share one PTY registry entry
  • expose local replay, live output, ordered input, resize, detach, policy recheck, and connection/session-limit behavior
  • cap decoded local output at one chunk outside the existing one-megabyte encoded queue
  • bound local input at exactly 256 queued-or-in-flight calls and 1 MiB admitted copied bytes, reserving before copying
  • reserve exactly 64 calls/256 KiB for browser input beyond the complete local budget; browser-only pressure waits without polling and wakes on accounting release, connection death, or session closure
  • normalize a duplicated PTY master to permanent nonblocking mode before Go wraps it, preserving runtime pollability on Linux/Darwin without later File.Fd calls
  • route browser and local input through exactly one session worker, with 25 ms POLLOUT waits and a 4096-byte round-robin fairness quantum

Verification

  • focused scheduler/backpressure and real-PTY tests: -count=20
  • focused scheduler/backpressure and real-PTY race tests: -count=5
  • mise exec -- go test ./...
  • mise exec -- go test -race ./...
  • mise exec -- go vet ./...
  • mise exec -- govulncheck ./... (0 reachable vulnerabilities; one required-module vulnerability is not called)
  • GOOS=darwin GOARCH=amd64 CGO_ENABLED=0 mise exec -- go test -exec /bin/true ./... -run "^$"
  • GOOS=darwin GOARCH=arm64 CGO_ENABLED=0 mise exec -- go test -exec /bin/true ./... -run "^$"
  • git diff --check

Darwin was cross-compiled, not runtime-tested locally. The Linux real-PTY gates run on a raw/no-echo slave and a master filled to actual EAGAIN.

Deliberate gate failures

All mutations below were run one at a time and reverted before the current head.

Initial gates:

  • changed TerminalSessionInfo.ProjectID from project_id to projectId; the literal DTO test failed on the missing snake_case key and decode mismatch
  • changed local session ids to unique timestamp suffixes; the reattach test failed with local reattach created a duplicate shell
  • removed the project root from the local attach header; the policy control failed on the allowed attach
  • changed the local slow-client deadline to one hour; the isolation test failed with stalled local client was not detached at the write deadline
  • changed the decoded local output capacity from one chunk to two; the test failed with local decoded output queue capacity = 2, want one bounded handoff

Admission and lifecycle gates:

  • removed the detach check from the former per-client input loop; the test failed with local input worker did not release queued input after detach
  • relaxed the byte budget by one byte; the test failed with input beyond byte budget error = <nil>, want ErrTerminalInputBackpressure
  • skipped local reattach size application; the test failed with reattached PTY size = 80x24, want 100x40
  • changed the shared session-limit comparison from >= to >; the test failed with local session limit error = <nil>
  • changed the call-admission comparison from >= 256 to > 256; the test failed with accepted local input calls = 257, want exact bound 256
  • moved the implementation copy ahead of admission; the test failed with implementation copies = 769, accepted calls = 256; rejected calls allocated
  • removed the admission mutex from detach; the test failed with detach passed a Write already holding admission
  • returned retryable backpressure for oversized input; the test failed with oversized local input error = terminal input backpressure, want ErrTerminalInputTooLarge

Nonblocking scheduler and real-PTY gates:

  • bypassed nonblocking pollable normalization; TestPTYMasterRemainsNonblockingAcrossTerminalIOCTLS failed with register pty with runtime poller: file type does not support deadline
  • restored a direct terminal Write and removed the raw helper; TestTerminalInputHasOneRawPTYWriterAndOneScheduler failed with direct terminal_sessions.go Write call and unix.Write PTY helpers = 0, want exactly one
  • started a second scheduler worker; TestTerminalInputStartsExactlyOneWorkerAndCloseDrainsAccounting failed with panic: close of closed channel
  • removed session-close draining; the close gate failed with nonzero session/local call and byte accounting
  • stopped canceling detached local requests; TestRealPTYDetachReleasesMaximalLocalInputAndBrowserProgresses failed with input accounting session=1/1048576 local=1/1048576, want 0/0
  • changed the fairness quantum to 8192 bytes; TestRealPTYInputFairnessAt4096ByteQuantum failed with browser marker followed 8192 local bytes, exceeds exact 4096-byte quantum

Browser-pressure gates:

  • changed the browser reserve to 256 KiB - 1; TestTerminalInputExactSchedulerBounds failed with scheduler bounds = 320 calls/1310719 bytes, want 320 calls/1.25 MiB
  • rejected browser input instead of waiting; TestBrowserInputFullCapacityWaitsThenResumes failed with browser submission did not enter capacity wait
  • initialized the scheduler channel with the old 256-call capacity; TestTerminalInputExactSchedulerBounds failed with initialized scheduler channel capacity = 256, want 320\n- moved session-local reservation after the copy; TestLocalTerminalWriteReservesAggregateBeforeCopyAcrossClients failed with aggregate reservation before copy total=0/0 local=0/0\n- changed the combined call threshold from 320 to 321; TestTerminalInputExactSchedulerBounds failed with scheduler bounds = 321 calls/1310720 bytes, want 320 calls/1.25 MiB\n- removed connection cancellation from the between-chunk capacity wait; TestBrowserLargeFrameChunkWaitCancellationDrainsAccounting/connection failed with between-chunk cancellation did not unblock submission\n- skipped rollback when a reserved local request found publication closed; TestReservedLocalPublicationRollsBackWhenSessionCloses failed with session ledgers after close = [1 1048576 1 1048576 0 0], want all zero\n- shrank the production scheduler channel to 319; TestReservedLocalInputOwnsFutureSchedulerChannelSlot failed with production scheduler channel capacity = 319, want 320 future-slot invariant

@nicodes
nicodes marked this pull request as ready for review August 24, 2026 20:20
@nicodes

nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Initial high-risk review wave

Pinned head: 978fc779460ae3175b59058b39712202f62088b6

Risk: high because this changes shared PTY I/O, policy enforcement, session lifetime, and multi-client concurrency.

Reviewers:

  • GPT-5.6 Luna: NEEDS ATTENTION
  • Kimi K3-256K: could not start because its billing-cycle quota was exhausted
  • Grok 4.6 replacement, isolated on the same unchanged head: APPROVE

The replacement preserves independent contexts and a different named model family; the runtime cannot independently prove provider-level model-family diversity beyond those identities. No code changed during the wave.

Consolidated findings and dispositions

  • BLOCKING: TerminalClient.Write is synchronous and detach cannot cancel a PTY write blocked behind a non-reading slave. Concurrent callers can also queue unboundedly on inputMu and block browser input. Accepted. Add a reachable regression first, then make local input bounded and detach-aware without killing the shared shell.
  • BLOCKING TEST GAP: the mixed browser/local control proves shared output and local input, but not browser input into that same PTY; maxTerminalSessions is not exercised through the local path. Accepted. Add both controls.
  • FOLLOW-UP: Output is not closed and callers must select on Done; the current comment states only one of the reasons Done closes. Accepted in this PR. Document all closure semantics precisely.
  • FOLLOW-UP: dimensions passed to AttachTerminal apply on create but not reattach unless the caller separately resizes. Accepted in this PR. Make attach semantics explicit and test them.

No other findings were consolidated. All accepted items will be fixed in one remediation change, then the changed head will rerun full gates and return to both recoverable reviewer contexts for verification.

Verdict: NEEDS ATTENTION

@nicodes

nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Remediation verification wave

Pinned head: 3a84e2d9ca99ee1f35678d2d9e0d1e601c45a948 (all exact-head checks green)

  • GPT-5.6 Luna: NEEDS ATTENTION
  • Grok 4.6 replacement context: APPROVE with one follow-up

Prior findings

  • Mixed browser/local input and session-limit test gap: resolved.
  • Output/Done lifecycle contract: resolved.
  • Reattach dimensions: resolved.
  • Bounded/detach-aware local input: improved but not fully resolved.

New evidence and dispositions

  • BLOCKING: Write copies before reserving the 1 MiB budget, so concurrent rejected calls can allocate outside the claimed bound; POLLOUT is checked before session.inputMu, so readiness can be consumed by a sibling before the local blocking write takes the shared lock. Accepted. Reserve before copying and revalidate writability while holding the shared lock, with a maximum 25 ms lock hold per failed probe.
  • BLOCKING TEST GAP: the 256-call bound is not directly exercised, and the PR description does not yet report the remediation gates deliberately broken and restored. Accepted. Add a reachable count-bound gate and update the PR evidence after all mutations are reverted.
  • FOLLOW-UP: Write can race detach and report success for input the worker then discards. Accepted in this PR. Serialize acceptance with local detach so a successful return means acceptance happened before detach.
  • FOLLOW-UP: oversized input returns retryable ErrTerminalInputBackpressure, causing a retry loop that can never succeed. Accepted in this PR. Add a distinct non-retryable error and test.

These are based on new remediation-code evidence. They do not reopen resolved initial findings. The fixes will be batched before the next exact-head verification.

Verdict: NEEDS ATTENTION

@nicodes

nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Final-verification wave on 7647c36

  • GPT-5.6 Luna: NEEDS ATTENTION
  • Grok 4.6 replacement context: APPROVE

Both reviewers agree that admission accounting, exact 256-call/1 MiB bounds, detach ordering, oversized errors, mixed input, session limits, reattach size, and published gate evidence are resolved.

They disagree on one platform semantic. The implementation uses the portable 512-byte PIPE_BUF floor after a locked POLLOUT check, but PIPE_BUF guarantees pipes/FIFOs, not PTY masters. A blocking PTY write can therefore still enter the kernel while holding session.inputMu, and the current regression uses a pipe rather than the production PTY.

Disposition:

  • BLOCKING: actual nonblocking guarantee after PTY readiness check. Accepted conservatively. Replace the assumption with nonblocking-by-construction input or a shared bounded writer design; do not merge based on pipe semantics.
  • BLOCKING TEST GAP: production PTY pressure is not exercised. Accepted. Add a real-PTY control for Linux and keep Darwin claims limited to what exact CI/runtime evidence establishes.

This is new platform-scope evidence, not a reopened admission finding. If a portable fix requires changing the browser input architecture, that change will be reviewed as part of this same high-risk PR rather than hidden behind the existing test.

Verdict: NEEDS ATTENTION

@nicodes

nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Verification wave on a950de6

  • GPT-5.6 Luna: APPROVE with one follow-up
  • Grok 4.6 replacement context: NEEDS ATTENTION

Both reviewers agree the prior blocking PTY-write semantics and pipe-only evidence gap are resolved by the permanent nonblocking master, sole session writer, and real-PTY gates. Admission accounting, detach ordering, close drain, fairness, typed errors, and resize/pollability remain resolved.

Consolidated finding

  • BLOCKING: a legal 1 MiB local input request consumes the entire shared session byte budget. The next browser keystroke makes submitInput return false, and attach treats resource pressure as a fatal connection error. A browser-only burst can similarly disconnect on the 257th queued frame. Accepted. Preserve a bounded browser reserve beyond the local budget and make browser submission wait on released capacity/session-or-connection cancellation rather than detach. Local Write remains nonblocking and returns backpressure. Add controls for maximal local input plus browser admission and browser-only backpressure without disconnect.

This is a new scheduler admission-policy regression introduced by a950de6, not a reopened PTY safety finding.

Verdict: NEEDS ATTENTION

@nicodes

nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Verification wave on 38b55d6

  • GPT-5.6 Luna: NEEDS ATTENTION
  • Grok 4.6 replacement context: APPROVE

Both reviewers agree the accepted browser-disconnect blocker is resolved: maximal local input now leaves browser reserve, browser-only pressure waits without polling, wake/cancellation paths are synchronized, the production channel has 320 slots, and exact-head checks pass.

Consolidated findings

  • BLOCKING: session-wide local admission is still reserved after copyInput. Multiple local clients can each pass their per-client checks and allocate 1 MiB before all but one fail the aggregate 1 MiB session cap. Accepted. Reserve both per-client and session-local counters before copying, then roll back both on enqueue failure/close.
  • BLOCKING TEST GAP: add synchronized multi-client evidence that aggregate rejection makes no copy; fill the combined 256-local/64-browser call limits; exercise browser frames larger than 256 KiB for bounded ordered chunks and cancellation. Accepted.

The original browser-pressure finding remains resolved.

Verdict: NEEDS ATTENTION

@nicodes

nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Verification wave on cec1550

  • GPT-5.6 Luna: NEEDS ATTENTION
  • Grok 4.6 replacement context: APPROVE

Both reviewers agree the aggregate reserve-after-copy blocker, combined-bound gap, large-frame chunk gap, and original browser-pressure blocker are resolved. Exact-head checks pass.

Consolidated findings

  • BLOCKING TEST GAP: deterministically pause a local Write after all ledgers reserve but before publication, close the session, and prove failed publication rolls all ledgers back exactly once, wakes a browser capacity waiter, and does not deadlock. Accepted.
  • BLOCKING TEST GAP: with 319 requests active/queued, pause the 320th after reservation, prove a 321st local request is rejected without copying, release the 320th, and prove its pre-owned channel slot publishes successfully and drains. Accepted.

No implementation defect was identified on this head. This remediation is test-only and bounded to the two reachable concurrency branches above.

Verdict: NEEDS ATTENTION

@nicodes

nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Final verification wave on 5cfd212

Risk: high. Two resumed independent reviewer contexts inspected the exact same green head.

  • GPT-5.6 Luna: APPROVE
  • Grok 4.6 replacement context: APPROVE

Both verified the delta is test-only and the implementation is unchanged.

Resolved evidence:

  • close after a 1 MiB local reservation but before publication wakes a browser capacity waiter, returns local backpressure, cannot pass the held local admission lock, rolls all six session ledgers and both local ledgers to zero exactly once, and does not deadlock
  • with 255 local plus 64 browser requests published, the reserved 320th request owns the remaining production channel slot, a 321st local request is rejected without copying, the 320th publishes successfully, and close drains all accounting
  • exact-head actionlint, ci, and darwin checks pass
  • focused tests passed 20 runs and focused race tests passed five runs; full tests/race/vet/vulnerability scan and Darwin amd64/arm64 compile-only checks pass locally

No BLOCKING, BLOCKING TEST GAP, or FOLLOW-UP findings remain.

Verdict: APPROVE

@nicodes

nicodes commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Merge-gate decision: the repository owner explicitly confirmed that the published approval status from the two independent external reviewer contexts is sufficient. GitHub cannot create a formal review because the sole collaborator is the PR author. Proceeding with the exact-head dual APPROVE record at #71 (comment); no review requirement is configured in branch protection, and all required checks remain green.

@nicodes
nicodes merged commit f5cd3ed into main Aug 24, 2026
4 checks passed
@nicodes
nicodes deleted the feature/504-local-terminal-client branch August 24, 2026 23:31
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