ST4 worker: follow-ups deferred during review

[pdettori]

Follow-ups from the ST4 worker (#87, PR #172) that were seen, judged, and deliberately
not fixed. None affects the acceptance battery or runtime correctness on the paths the
harness exercises today.

Filed as its own issue rather than a note in the repo, because an in-repo list went stale
within one commit: it was generated from an implementation ledger that recorded findings
as they were raised and never reconciled against the code after the final review wave,
so it advertised three blockers that were already fixed. Everything below was audited
against feat/st4-go-worker at review time.

Item numbers are stable and never reused. Resolved items move to Resolved at the
bottom keeping their original number, because commit messages and a test comment
(packages/knative-server/test/worker-deployment.test.ts) cite them as "#173 item N".
Last reconciled against main at dd77aca, 2026-09-08 — each open item below was
re-verified in the tree, not just re-read. Four items remain open: 2, 4, 9, 10.
None is a correctness defect: 2 needs a both-ends wire decision, 4 is a pure refactor, and
9 and 10 are small. Note item 4 is now more worth doing than when filed — PRs #229 and #230
both touched accept's signature, which is the plumbing item 4 is about.

Worth doing, in rough priority order

• 2. gRPC MaxCallRecvMsgSize is unconfigured, so a base64 write payload above the
  4 MiB default kills the whole stream rather than one exec, triggering a reconnect.
  Needs a decision on both ends plus a documented max write size — relay-side
  coordination, which is why it is not in the PR.
  Still open: no message-size option is set anywhere in remote-worker/ or
  packages/sandbox-relay/.

• 4. Unify the session's state plumbing. abortReq/finish close over inflight/mu
  while recvLoop/accept take the map and a *sync.Mutex as positional parameters —
  seven parameters, two of them raw synchronization primitives. For a file whose central
  safety property is "every access is under mu", one convention would make that locally
  checkable instead of requiring a whole-file audit. A small struct with methods collapses
  both signatures. Wanted specifically because this is the reference implementation
  other language ports get written against.
  Still open at loop.go:248 (recvLoop) and loop.go:278 (accept). Pure refactor —
  best folded into whichever change next touches that file.

Smaller

• 9. settle() is a fixed sleep in an otherwise poll-based test file. Justified: there
  is no exported observable for "the inflight slot has been released". An exported test
  hook would let it become a condition wait.

• 10. The images install findutils, which none of the harness's operations use.
  Still present in remote-worker/Dockerfile and remote-worker/Dockerfile.runtime.

Resolved

• 1. A dropped refusal left an exec unanswered. Done in PR fix(worker): reserve outbound capacity for terminal frames (#173 item 1) #230 (b0da405, refined in
  26c689b). The receive goroutine sent through a non-blocking trySend so it could never
  stall and miss an Abort, but every frame it sends is terminal and refusals are uncached,
  so a drop lost the caller's only answer — and it correlated with the overload that caused it.
  KubectlTransport has no default deadline while GrpcRelayTransport always applies 120s, and the battery cannot see it #182 had since made that stall up to 30 minutes (DEFAULT_EXEC_TIMEOUT_S), retiring the
  deferral's own "survivable because the harness timeout is dual-ended".
  The remedy named in this item is unsafe as written. "A priority channel, or a dedicated
  terminal-frame forwarder" both let a terminal frame overtake queued chunks for the same
  req_id, and spec §8 requires Chunk* then End. Two reachable cases: a cache-hit replay
  overtaking the original's still-queued chunks, and a colliding refusal overtaking an earlier
  exec's frames under the same id — the harness settles on whichever lands first and discards
  the real output behind it. Safe priority needs per-req_id pending tracking.
  Shipped instead: reserved capacity in the single channel. outbound stays one FIFO (so it
  cannot reorder, by construction) and gains TerminalReserve (QueueCap/4 = 16) slots that a
  chunkSlots semaphore keeps chunk producers out of; the sender releases a slot when it
  takes a non-reserved frame, so the accounting measures channel residency rather than
  tracking Send latency. Exhaustion is then a genuinely different condition — nothing draining
  at all — so accept returns ErrEgressWedged, recvLoop propagates it, and Serve returns
  it for main.go to re-dial, with dedup answering redeliveries (§5, §6.2). A dropped cache-hit
  replay escalates too: the harness redelivers precisely because it never got the first answer.
  Guarded by TestBusyRefusalSurvivesAChunkBacklog (defect + FIFO),
  TestExhaustedReserveEndsTheSession, and a compile-time assertion that the reserve cannot
  silently become 0. No wire or proto change — §8 is enforced here, not amended.
  Also fixed a teardown wedge that predates this item: a Send that blocks (rather than
  fails) parked the sender, so a producer waiting for room waited forever and wg.Wait() never
  reached zero, after which Serve could never return — the one thing that makes main.go
  re-dial. enqueue now gives up on connCtx. TestSendFailureDoesNotWedgeTeardown missed it
  because a failing Send lets the sender keep draining. Pinned by
  TestBlockedSendDoesNotWedgeProducers.

• 3. SANDBOX_TOKEN was a literal env value in worker-deployment.yaml, filled by
  sed in deploy-incluster.sh, so it landed in the Deployment spec, oc describe, and
  any GitOps mirror. Done in PR fix: open-issue triage, first session (#190, #191, #192, #182, #173 item 3) #226 (e33bf3e and follow-ups): the token now arrives
  via secretKeyRef on the worker Deployment, the OCP overlay, and both deploy scripts,
  and both relay and worker are restarted after a token rotation, since env from a
  secretKeyRef is resolved only at pod start. worker-example.yaml keeps its literal
  dev-token on purpose. Guarded by
  packages/knative-server/test/worker-deployment.test.ts, which fails if the literal
  returns or if the secretKeyRef stops matching the Secret name/key the deploy script
  creates.

• 5. Most tests never joined the Serve goroutine. Done in PR fix(worker): #173 items 5, 6, 8 — build constraint, joined Serve goroutines, activity-based drain grace #229 (e8aae74).
  loop_test.go gained a serve() helper that starts Serve and registers the cleanup
  which closes the stream and waits for the return; contract_test.go's
  attachCancellable now joins as well as cancels, and the SH_LIVE_RELAY gate joins too.
  fakeStream.close() became idempotent, since the cleanup always closes as well.
  Measured rather than assumed: injecting this item's exact deadline — deleting the
  cancelConn() before close(queue), so the heartbeat producer never returns — left
  14 of loop_test.go's 15 tests passing; with the join it fails 17. A second fault
  that cancellation cannot mask (close(outbound) removed) confirmed the contract-test
  join: all 11 catch it, where before they passed. No production code changed.
  This item's premise was wrong on one point: it said "Two of them observe teardown".
  Only one did. TestSendFailureDoesNotWedgeTeardown looks like the second, but a failing
  Send makes the sender call cancelConn() itself, ending the heartbeat producer as a
  side effect, so it passed even with the deadlock injected. Its comment now says what it
  really covers.

• 6. Drain-watchdog grace was wall-clock, not activity-based. Done in PR fix(worker): #173 items 5, 6, 8 — build constraint, joined Serve goroutines, activity-based drain grace #229
  (c5f17c9, refined in afc83eb). drainGrace is now a quiet period: any read returning
  bytes defers the force-close, tracked by one atomic counter that drain bumps before
  delivery. The watchdog moved out of Run into a package-level watchDrain, so both
  behaviours are unit-testable at millisecond timings.
  Two deliberate departures from this item as written. It was not as narrow as "the run
  is already out of budget" implies — a python3 writer that os.setsid()s out of the
  process group and ticks every 400 ms delivered 7 of 10 ticks, truncated at exactly
  3.01 s = 1 s timeout + 2 s grace. And "resetting the timer on read activity" is unbounded
  as stated, so a holder trickling forever would pin a pool slot and up to BufferCap —
  a slower form of the wedge the watchdog exists to prevent. Added drainCeiling = 30 s
  (15× the grace), verified by neutering it and watching only the ceiling test fail.
  Note the real teardown bound: progress is sampled at timer expiry, not observed per
  read, so the close lands between 1× and 2× drainGrace after the last byte — up to
  ~4 s. Pinned by TestWatchDrainClosesAtTwiceGraceAfterTheLastRead. No wire or spec
  change; the watchdog appears in no spec.

• 7. The memory coupling is documented but unenforced. BufferCap × 2 streams ×
  MaxConcurrent must fit the pod limit; both sides carried the arithmetic in a comment,
  but nothing stopped a future WORKER_MAX_CONCURRENT entry in the Deployment from
  invalidating the 256Mi limit silently. Done before this issue's first triage pass
  (65160bf, strengthened by a1ba25b, both 2026-08-28): the same
  worker-deployment.test.ts reads BufferCap from runner.go, DefaultConcurrency from
  loop.go, and the manifest's WORKER_MAX_CONCURRENT override if one exists, then
  asserts the pod memory limit covers 2 × concurrency × BufferCap. It throws loudly
  rather than skipping if a constant is reformatted out of reach.

• 8. No //go:build unix constraint. Done in PR fix(worker): #173 items 5, 6, 8 — build constraint, joined Serve goroutines, activity-based drain grace #229 (7aa1bad). Both
  syscall.Setpgid and syscall.Kill now sit behind //go:build unix.
  This needed one step past the remedy this item prescribed. "Just the tag" would have
  turned two errors about a struct literal into build constraints exclude all Go files in …/internal/exec — better located, but still not the "clear unsupported platform message"
  the item asks for. So the !unix side is a refusal rather than a second implementation:
  Run returns "remote-worker is unix-only: <GOOS> has no equivalent of the process-group
  isolation (Setpgid/Kill) that abort and timeout depend on" before spawning anything, on the
  grounds that a runner which cannot reliably kill what it spawned has no correct behaviour
  to offer. Still no platform-specific logic — a tag and a refusal.
  Guarded by TestModuleCrossCompilesForNonUnix, which cross-compiles the module for
  GOOS=windows (~1.1 s) rather than asserting on the constraint's text.

Platform note, not a follow-up

TestRunReturnsWhenPipeHolderEscapesGroup needs setsid to detach a pipe holder from the
process group, so it skips on macOS and runs on Linux CI. It is the test that hangs
rather than fails if the drain watchdog regresses, so its coverage is real — just not on a
developer's Mac.

Its sibling from item 6, TestSlowDrainKeepsTrailingOutput, needs the same detachment but
gets it from python3 -c 'import os; os.setsid()', which works on macOS too — so that one
runs everywhere. Prefer that form for any future test needing a process outside the group;
setsid(1) simply does not exist on macOS, and a test that silently skips looks exactly
like a test that passes.

Assisted-By: Claude Code