fix: close factory, config, and playground liveness gaps - #1024
Conversation
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review ? 7fe88d3ff (#1018)
This commit replaced module-local synthetic-comment WeakMaps with a versioned realm registry, covered CJS?ESM and duplicate physical package copies in both directions, retained frozen-node behavior, and documented cross-version limits.
The review found one P2 compatibility regression: an unconditional registry write made first load throw in a hardened realm whose globalThis is non-extensible. The finding was accepted. Commit 4a84a9647 adds a module-local fallback, a fresh-process non-extensible-global regression test, and the corresponding documentation boundary.
Everything else in the commit's declared surface was sound: cross-copy reads, printing, clears, reverse-direction transfer, structural node compatibility, and test non-vacuity. Focused factory package/build/test validation passed.
| leading: new WeakMap<object, SynthesizedComment[]>(), | ||
| trailing: new WeakMap<object, SynthesizedComment[]>(), | ||
| }; | ||
| Object.defineProperty(globalThis, SYNTHETIC_COMMENT_STORES, { |
There was a problem hiding this comment.
P2: Object.defineProperty(globalThis, ?) throws when a hardened realm has made the global object non-extensible. The previous module-local WeakMaps loaded successfully there. Preserve the realm-shared registry where installation is possible, but fall back to local WeakMaps when it is not, and cover a first-load non-extensible-global process. Resolved by 4a84a9647.
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review ? 4a84a9647 (#1018 correction)
CLEAN. The hardened-realm correction safely absorbs registry-installation failure and preserves the historical per-module WeakMap behavior without mutating nodes. Normal realms still share the first compatible registry across CJS, ESM, and physical copies; hardened realms retain frozen-node comment storage within each copy, with the cross-copy limitation documented.
Verification passed: @ttsc/factory build, scripts/ci/factory-package.test.cjs (including the fresh non-extensible-global child process), the full factory test package, formatting, and git diff --check.
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review ? e8e8da569 (#1019)
CLEAN. The lint evaluator now classifies ETIMEDOUT and ENOBUFS before their shared SIGTERM, preserves distinct launch/signal/nonzero/success paths, and keeps child stream forwarding intact. The helper stays internal and the regression matrix is non-vacuous.
Verification passed against real Node spawnSync result shapes, @ttsc/lint build, the full plugin test subtree, test-package type checking, formatting, and git diff --check.
A later Overall Self-Review found that a single enormous stderr line could still duplicate nearly the process buffer into the parent exception; correction bb2690de6 added the independent 8 KiB character bound and a 100,000-character regression case.
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review ? 1efc874b0 (#1022)
The implementation correctly placed one finite deadline across fetch and JSON-body phases, forwarded the internal abort signal to injected/default fetch, retained healthy URL-keyed single-flight behavior, evicted rejected entries by identity, surfaced phase/URL/caller cause, and separated WASM runtime boot from retryable source-pack mounting.
The review found one P2 documentation gap: the package README and maintainer website README had changed, but the canonical public Playground guide did not state the new 30-second/shared-cancellation/retry/runtime-reuse contract. The finding was accepted and resolved by d944e35ee.
Focused source-pack, worker-service, full 42-case Playground, build, type-check, formatting, and diff-integrity verification passed.
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review ? d944e35ee (#1022 documentation correction)
CLEAN. The canonical Playground guide now states the exact implemented contract: one 30-second bound across response and body, URL-keyed single-flight, cancellation of the shared attempt by any caller/deadline, rejection eviction, next-request mount retry, and reuse of the already-running WASM runtime.
The text matches implementation and package documentation, and the MDX passes formatting and git diff --check.
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review ? ce74753bc (#1023)
This commit bounded executable descriptor evaluation at 60 seconds and 16 MiB, classified timeout/output/launch/signal/nonzero/missing/invalid/success paths, bounded descriptor JSON, retained finally cleanup, exercised API/CLI/LSP propagation with a fake ttsx, and documented the plugin-author contract.
The review found one P2 diagnostic compatibility regression: the prior nonzero path used stderr || stdout, but the new helper consulted only stderr, losing stdout-only evaluator causes. The finding was accepted. Commit bb2690de6 restores stderr-first, nonblank stdout fallback and applies the same 8 KiB/5-line bound to either stream, with pure and end-to-end regression coverage.
All other classification, cleanup, propagation, actual-ttsx happy-path, formatting, and diff-integrity checks passed. The broader Windows project subtree had one pre-existing symlink-creation EPERM; the exact changed tests passed independently.
| ); | ||
| } | ||
| if (result.status !== 0) { | ||
| const reason = descriptorFailureReason(result.stderr); |
There was a problem hiding this comment.
P2: this drops the previous stderr || stdout behavior. A ttsx evaluator that reports its nonzero cause only on stdout is reduced to an exit code. Prefer nonblank stderr, otherwise use a bounded stdout fallback. Resolved by bb2690de6, with both helper-level and fake-ttsx end-to-end coverage.
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review ? bb2690de6 (integrated evaluator-diagnostic correction)
CLEAN. Both evaluator helpers now cap a giant single diagnostic line at 8,192 characters before applying the final-five-nonempty-lines rule. Descriptor failures prefer meaningful stderr and fall back to stdout only when stderr is empty/whitespace, with the same bound. The real descriptor call path passes both streams, and the fake-ttsx end-to-end test proves stdout-only nonzero diagnostics survive.
Verification passed: ttsc and @ttsc/lint builds, both descriptor tests, the lint classification test, explicit boundary probes, changed-file formatting, and git diff --check.
samchon
left a comment
There was a problem hiding this comment.
Overall Self-Review ? finding round 1 (ce74753bc)
A fresh base-to-head review covered all 21 changed files and their package, test, CI, documentation, cache/retry, failure-propagation, compatibility, Windows/POSIX, and packaging consequences. Two sound P2 improvements survived reproduction:
- The final-five-lines rule did not bound a single enormous stderr line, so a process that reached the 16 MiB capture limit could duplicate almost all of it into the parent exception.
- The descriptor refactor lost the previous nonblank
stderr/stdoutfallback, reducing stdout-only nonzero diagnostics to an exit code.
Both findings were corrected in bb2690de6 with an independent 8 KiB character cap, stderr-first bounded stdout fallback, helper boundary cases, and a fake-ttsx end-to-end stdout-only failure case. The Overall Self-Review restarted from the complete base-to-new-head surface as required.
| stderr: string | null | undefined, | ||
| ): string { | ||
| const lines = (stderr ?? "") | ||
| .split(/\r?\n/) |
There was a problem hiding this comment.
P2: limiting the number of lines does not limit one giant line. spawnSync can capture up to 16 MiB, and this exception can duplicate nearly all of it. Slice to a small character suffix before selecting the final nonempty lines. Resolved by bb2690de6 with a 100,000-character regression case.
| ); | ||
| } | ||
| if (result.status !== 0) { | ||
| const reason = descriptorFailureReason(result.stderr); |
There was a problem hiding this comment.
P2: the previous loader selected stderr || stdout; this refactor consults only stderr. Preserve nonblank stderr priority and use a bounded stdout fallback for stdout-only evaluator failures. Resolved by bb2690de6 in both the pure classifier and fake-ttsx integration path.
samchon
left a comment
There was a problem hiding this comment.
Overall Self-Review ? finding round 2 (bb2690de6)
The required fresh full review found one cross-platform liveness gap in both synchronous evaluators. Supplying timeout without killSignal uses Node's default SIGTERM. Node documents that spawnSync() still waits indefinitely when a POSIX child handles SIGTERM and does not exit, so the stated finite descriptor bound and lint evaluator anti-hang invariant were not actually hard bounds.
The finding is accepted. Both evaluators will use SIGKILL, and a finite child probe that traps SIGTERM will lock the shared process-option contract. After correction, formatting and focused validation, the complete Overall Self-Review will restart again from the new exact head.
| TTSC_PLUGIN_ENTRY: request, | ||
| }, | ||
| maxBuffer: PLUGIN_DESCRIPTOR_MAX_BUFFER_BYTES, | ||
| timeout: PLUGIN_DESCRIPTOR_TIMEOUT_MS, |
There was a problem hiding this comment.
P1 liveness: Node defaults killSignal to SIGTERM, and its spawnSync contract explicitly keeps waiting if the child handles SIGTERM without exiting. A descriptor can therefore still block setup indefinitely after the nominal 60-second timeout. Use a non-interceptable SIGKILL process-option contract and cover it with a finite signal-trapping child probe.
| // 60s cap so a runaway top-level await / infinite loop in the | ||
| // user's lint config can't hang the entire ttsc invocation. | ||
| timeout: 60_000, | ||
| timeout: CONFIG_EVALUATOR_TIMEOUT_MS, |
There was a problem hiding this comment.
P1 liveness: the same default-SIGTERM caveat applies to the lint config evaluator, whose comment and error contract say the 60-second cap prevents a runaway config from hanging the invocation. Share an explicit SIGKILL option with its classification test.
samchon
left a comment
There was a problem hiding this comment.
Development-phase discovery ? deferred to the mandatory post-merge cycle
Full local validation surfaced one baseline candidate outside this frozen implementation set. On Windows, pnpm run build reached the cross-platform Go SDK extraction for ttsc-darwin-arm64 and failed:
tar (child): Cannot connect to D: resolve failed
gzip: stdin: unexpected end of file
tar: Child returned status 128
Primary evidence:
Get-Command tarresolvesC:\Program Files\Git\usr\bin\tar.exe(GNU tar 1.35), ahead ofC:\Windows\System32\tar.exe.scripts/build-platform-package.cjsinvokes ambienttar -xzf <absolute Windows archive path> -C <absolute Windows extraction path>.- GNU tar interprets the unqualified
D:archive operand as remote archive syntax unless forced local; the downloaded archive had already passed the repository's checksum verification. - Every changed package build and focused/current-head suite passes. GitHub's cross-platform release build runs on Linux, so this baseline portability defect does not invalidate the frozen #1018/#1019/#1022/#1023 PR.
Per the solo-campaign phase boundary, this is not retroactively inserted into the already frozen cycle. Its exact reproduction and environment provenance are preserved in the campaign ledger. After #1024 merges, the required new whole-repository discovery saturation will reopen it against merged master, search issue/PR history, compare GNU tar and Windows bsdtar behavior, and adjudicate the portable extraction owner before publication or implementation.
There was a problem hiding this comment.
Individual Self-Review - correction 6b0a2cf8f
Verdict at this commit: merge-blocking finding; corrected by a8e7bf6d9.
Scope and evidence
I reviewed the exact correction commit that replaced the default termination signal with SIGKILL for both lint-config and plugin-descriptor evaluation. I traced the complete process topology instead of stopping at the immediate spawnSync call:
lint/plugin parent -> ttsx wrapper -> runPreparedEntry runtime child -> user config/descriptor
A POSIX reproduction with the actual two-level topology confirmed the gap: the outer parent returned ETIMEDOUT, but a marker scheduled by the nested runtime was still written later. Killing the ttsx wrapper therefore did not establish the claimed evaluator deadline.
Findings
- P1 - the timeout and output cap terminate only the outer ttsx wrapper, not the process that executes user code. The wrapper uses a second synchronous spawn for the actual runtime. A timeout or
ENOBUFSat the parent can kill the wrapper while leaving that runtime alive on POSIX. This defeats both liveness and isolation: timed-out config/descriptor code can continue side effects after the caller has failed. - P3 - two regression-test docblocks still described timeout/max-buffer failures as attaching
SIGTERM, although this correction configuredSIGKILL. The assertions were unaffected, but the rationale had become stale.
Resolution now present at the PR head
Commit a8e7bf6d9 carries an absolute semantic deadline, max-buffer limit, and private status fd through the wrapper; runPreparedEntry scrubs the protocol from user code, applies SIGKILL and the remaining deadline to the actual runtime child, and reports ETIMEDOUT/ENOBUFS to the original parent. It also adds real-ttsx regressions proving that neither a deadline-bound nor output-overflowing runtime reaches a delayed side effect, while ordinary ttsx stream forwarding remains unchanged. The stale docblocks were corrected there as well.
This review records why 6b0a2cf8f was not sufficient on its own; approval remains contingent on the separate review and CI validation of a8e7bf6d9.
| export const CONFIG_EVALUATOR_MAX_BUFFER = 16 * 1024 * 1024; | ||
| export const CONFIG_EVALUATOR_TIMEOUT_MS = 60_000; | ||
| export const CONFIG_EVALUATOR_PROCESS_OPTIONS = Object.freeze({ | ||
| killSignal: "SIGKILL" as const, |
There was a problem hiding this comment.
P1 (merge-blocking at this commit): SIGKILL makes this particular spawnSync return, but this process is only the ttsx wrapper. runPreparedEntry creates the Node process that actually executes the config/descriptor, and killing the wrapper does not kill that nested runtime on POSIX. The semantic deadline and output cap must be applied to the runtime child itself, with its cause propagated back to this parent.
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review - correction a8e7bf6d9
Verdict at this commit: one P2 test-adequacy finding; corrected by 764b5a57f.
Scope
The exact correction was traced across both parent evaluators, the private absolute-deadline/max-buffer/status-fd protocol, runTtsx, the actual runtime child, stream forwarding, environment scrubbing, cleanup, and both real-ttsx regressions.
The implementation correctly moved the hard bound onto the Node process that executes user config/descriptor code. Focused validation passed for both nested causes, parent classification, ordinary ttsx execution, preload/argv compatibility, executable lint-config stream forwarding, TypeScript checks, formatting, and diff integrity. A separate Node 24 Alpine probe also confirmed that a nested runtime trapping SIGTERM is terminated by SIGKILL, with ETIMEDOUT propagated through fd 3 and wrapper status 1.
Finding
P2 - the committed deadline regression did not itself install a SIGTERM handler in the actual runtime fixture. Replacing the inner killSignal: "SIGKILL" with Node's default SIGTERM would still terminate that fixture and leave the test green. The previous defect class was specifically that a POSIX evaluator may handle SIGTERM, so the regression needed to pin that condition at the real second-level runtime rather than only in direct process-option unit probes.
Resolution now present at the PR head
Commit 764b5a57f adds a typed process.on("SIGTERM", () => {}) handler before the runtime's ready marker. The existing absolute-deadline, fd-3 ETIMEDOUT, and late-marker assertions now prove that the actual evaluator cannot survive by handling the default signal. The exact focused test and test-package typecheck pass after the correction.
This review records the advisory finding against a8e7bf6d9; 764b5a57f receives its own mandatory correction review and exact-head CI.
| "", | ||
| ].join("\n"), | ||
| "src/main.ts": [ | ||
| 'const fs = require("node:fs");', |
There was a problem hiding this comment.
P2: This real runtime fixture must trap SIGTERM. Without that condition, changing the inner spawn from SIGKILL back to Node's default signal would still terminate the fixture and this regression would stay green, even though the original POSIX liveness bug returned. Install the handler before the ready marker and retain the late-marker assertion.
samchon
left a comment
There was a problem hiding this comment.
Individual Self-Review - correction 764b5a57f
CLEAN. The exact correction makes the real ttsx runtime fixture meaningfully distinguish graceful termination from a hard kill on POSIX.
- The runtime handles
SIGTERMand suppresses Node's default exit. - Its late marker is scheduled 750 ms after the semantic evaluator deadline, while voluntary exit is two seconds after that deadline.
- A regression to the default
SIGTERMtherefore leaves the runtime alive long enough to write the marker and fail the assertion; the configuredSIGKILLprevents it. - Windows accepts the handler and still exercises forced termination, while the POSIX lane supplies the behavioral distinction.
- A separate Node 22 Alpine probe confirmed that the handler survives
SIGTERMand exits only afterSIGKILL.
Validation passed on the exact commit: the focused real-ttsx regression (about 7.3 seconds), test-ttsc TypeScript checking, commit diff integrity, and a clean unchanged worktree.
samchon
left a comment
There was a problem hiding this comment.
Overall Self-Review - final clean round (764b5a57f)
CLEAN. This is the final fresh, solo, base-to-head review of the immutable
exact head 764b5a57ffcbceca791ead78851b8d24571ec8c9 against integrated baseline
1069333a98017d8d295a5f4106af842495a03c05. I read every hunk in all 24
changed files (1,718 insertions and 106 deletions), reopened all four issue
contracts, traced their downstream state and failure paths, reconciled every
Individual Self-Review and earlier Overall finding, and found no remaining
sound improvement.
Issue acceptance
- #1018 - cross-copy factory comments. The versioned
Symbol.for("@ttsc/factory.syntheticComments.v1")rendezvous shares the
leading and trailing WeakMaps across CommonJS/ESM entry instances and
physically duplicated compatible packages without putting metadata on AST
nodes. Read, print, reverse-direction write, replacement, and clearing all
cross the copy boundary. Frozen nodes remain valid. A non-extensible global
falls back to private stores rather than making package import or same-copy
comments fail, and the unavoidable legacy-copy/hardened-realm limitations
are documented. - #1019 - lint evaluator failures.
ETIMEDOUTandENOBUFStake
precedence over the shared termination signal; launch errors, external
signals, non-zero exits, and success remain distinct. The 60-second semantic
deadline and 16 MiB cap reach the actual nested runtime through a private,
scrubbed environment/fd-3 protocol.SIGKILLmakes the deadline hard even
when user setup handlesSIGTERM, while failure excerpts remain bounded. - #1022 - source-pack liveness. One URL owns one healthy in-flight request,
and the deadline covers both fetch and JSON body consumption. A caller abort
or deadline cancels the shared attempt, every rejection evicts it, and the
next compiler request retries the mount on the already-started WASM runtime.
Successful calls remain single-flight and the public timeout/signal policy is
exported and documented. - #1023 - descriptor evaluation liveness. TypeScript descriptor execution
has the same hard 60-second/16 MiB nested-runtime boundary and cause-specific
process classification. Descriptor JSON is independently size-bounded before
parsing; missing, malformed, oversized, stdout-only non-zero, and successful
outputs stay distinguishable. Temporary loader directories are removed on
API, CLI, LSP, evaluator, and later descriptor-validation failures.
Integrated consequence review
- The evaluator protocol is parent-owned, validates all three fields, and is
removed from the actual user's environment. Ordinaryttsxstill inherits
stdout/stderr and keeps its preload, argv, source-hook, status, and cleanup
behavior. - The process topology was checked through the outer lint/descriptor parent,
thettsxwrapper, and the Node runtime that executes user code. The real
runtime regression trapsSIGTERM, reportsETIMEDOUTon fd 3, and proves
no late side effect survives; the output-limit twin provesENOBUFSkills
the same runtime. - Playground boot separates the terminal/runtime promise from the retryable
mount promise. Concurrent compiler calls remain serialized, failed mounts do
not start a second Go runtime, successful mounts remain cached, and partial
pack data is never installed. - Windows/POSIX termination, process stdio shape, cache rejection and retry,
frozen objects, hardened globals, package entry formats, API/CLI/LSP error
propagation, generated declarations, package exports, and public
documentation were all included in the review. - The issue-closing commits still resolve exactly #1018, #1019, #1022, and
#1023 at HEAD. No revert or dropped implementation leaves a mechanical close
without a surviving fix.
Earlier findings and resolutions
The clean head contains every accepted correction from the preceding rounds:
bounded single-line evaluator excerpts and stdout fallback (bb2690de6), hard
outer evaluator termination (6b0a2cf8f), propagation of the boundary to the
actual nested runtime (a8e7bf6d9), and a regression fixture that makes
SIGTERM observably insufficient (764b5a57f). Each coherent implementation
or correction has its mandatory formal Individual Self-Review in this PR.
Exact-head verification
- Worktree clean;
git diff --check origin/master...HEADclean. pnpm formatpassed.ttsc,@ttsc/lint, factory, and playground builds/typechecks passed.- Factory package CJS/ESM/duplicate/hardened-global coverage passed.
- Lint and descriptor classifier suites passed.
- Real nested-runtime deadline and output-limit suites passed.
- Descriptor API/CLI/LSP propagation and temporary cleanup passed.
- Full playground and lint-plugin focused suites passed.
- GitHub exact-head CI: 61/61 successful, 0 failed, 0 pending.
testrun30221118070initially hit one unchanged macOS watch timing
failure (expected a project change after 0); the same lane was green on the
integrated baseline, neither the watch implementation nor test changed in
this PR, Ubuntu and Windows were green, and failed-job attempt 2 passed at the
identical SHA. All other workflows passed on that same SHA.
Deferred next-cycle candidate
The Windows ambient-GNU-tar absolute-drive-path failure discovered during
broad local validation is a baseline candidate, not a defect in this frozen
four-issue diff. Its primary evidence is preserved in the campaign ledger and
it will be reopened immediately in the mandatory post-merge full discovery
cycle. It does not weaken any acceptance claim above.
With both independent gates clean on the same immutable head, PR #1024 is ready
to leave draft and merge.
Campaign scope
This pull request delivers the frozen implementation set from the post-#1017 solo issue campaign against baseline
1069333a98017d8d295a5f4106af842495a03c05. The first complete discovery round accepted four independently reproduced issues; a fresh second whole-repository round produced no additional sound candidate, so implementation began only after the required empty-round gate.@ttsc/factorysynthetic-comment side-band state across compatible CJS/ESM and physical package copies while preserving frozen-node and hardened-realm behavior.Closing references live in the four coherent implementation commits; the claim and correction commits intentionally contain no closing keywords.
Integrated implementation
The current exact head is
764b5a57ffcbceca791ead78851b8d24571ec8c9.Factory comments now rendezvous through a versioned realm registry. If a hardened realm prohibits installing that registry, the module falls back to its historical private WeakMaps instead of failing to load. Package-level tests exercise bidirectional CJS/ESM and duplicate-copy reads, printing, clears, frozen nodes, and a fresh non-extensible global.
Lint config and plugin descriptor evaluation use 60-second semantic deadlines and 16 MiB output bounds. Failure classification checks
ETIMEDOUTandENOBUFSbefore signal state, preserves launch/signal/nonzero distinctions, bounds appended diagnostics to 8 KiB and five nonblank lines, retains descriptor stdout fallback, validates descriptor JSON size/shape, and always cleans temporary loader output.The evaluator bound reaches the actual second-level runtime spawned by
ttsx, not only the wrapper. Parents pass an absolute deadline, max buffer, and private fd-3 status channel;runPreparedEntryscrubs the protocol from user code, appliesSIGKILLand the remaining bound to the runtime child, and propagatesETIMEDOUT/ENOBUFS. Real-ttsx regressions prove that a SIGTERM-handling runtime cannot write a post-deadline marker and that an output-overflowing runtime is also terminated.Playground source-pack loading now accepts
signalandtimeoutMs, defaults to 30 seconds, covers response and body phases, preserves healthy URL-keyed single flight, and makes any caller cancellation/deadline cancel the documented shared attempt. Rejection is identity-evicted. Worker boot caches the Go/WASM runtime separately so a failed mount retries without starting a second runtime. Package, maintainer, and canonical website documentation state the contract.Review record
Every implementation or correction commit received exactly one pushed-head advisory Individual Self-Review. Findings were accepted and corrected for:
globalThisfactory loading;SIGTERMnot providing a hard POSIX bound;SIGTERM.The correction reviews for
4a84a9647,d944e35ee,bb2690de6, and764b5a57fare clean. The complete Overall Self-Review restarted after every integrated correction; its final report will be submitted as a formal review on this exact head after the exact-head matrix completes.Validation
Local focused validation on the integrated code includes:
ttsc,@ttsc/lint,@ttsc/factory, and@ttsc/playground;The broad local project subtree has one unrelated Windows symlink-creation
EPERMwhen Developer Mode is unavailable; all exact changed tests pass independently. GitHub exact-head CI is the merge authority.Next campaign cycle
A Windows full-build run exposed a baseline GNU tar portability candidate outside this frozen set: ambient Git-for-Windows GNU tar treats an absolute drive-letter archive operand as remote syntax. The exact failure, tool provenance, and a successful relative-path control are recorded for the mandatory post-merge discovery cycle. It is deliberately not retrofitted into this already frozen pull request.