Raise memory-default gate file concurrency to 8 behind a testable policy by tnunamak · Pull Request #83 · PDP-Connect/data-connect

tnunamak · 2026-09-09T17:25:14Z

The server test gate runs two test files at a time. On the same 1,033 files and the same tree, eight at a time finished in 141 seconds instead of 352, with identical failures. The limit was one inline expression in the runner, so nobody could see what a given machine would do without working it out by hand.

This moves the decision into reference-implementation/scripts/file-concurrency.ts: a function of CPU count, selected file count, operator override, and storage profile. The memory profile now allows 8, the Postgres profile stays at 2. The result is clamped by CPU count and file count and floored at 1, so a 4-CPU hosted runner gets 4. A PDPP_TEST_CONCURRENCY override is parsed with parseInt as before and is not clamped.

Five defects that stayed hidden while the gate was slow are repaired here, each in its own commit: a background upload-validation task that outlived its test and wrote into the next one's database, a 150 ms no-output allowance in the executor timeout test, a console build that ran silently inside a test and tripped the no-output watchdog, and writer-path teardowns that closed the database while deferred index work was still running, and a wall-clock race in those same tests, where a deliberate gate hold outran the production two-second admission budget on slow runners, so the test file now sets PDPP_INGEST_LOCK_WAIT_MS above any deliberate hold. Two of those repairs are the commits from #67 and #68, carried here unchanged.

Verify: node --test --import tsx reference-implementation/scripts/run-tests-file-concurrency-cap.test.ts calls the policy and asserts the worker count for both profiles, CPU and file clamps, and the override. The 141-versus-352 measurement is from one 24-core machine; hosted runners are clamped lower and were not measured.

Assisted-by: AI

The manual-upload route answers 202 and validates in a detached background task (setImmediate(() => validateAndStageArtifact(...)), see server/routes/ref-manual-upload-draft-connection.ts:1528) that nothing owns, awaits, or cancels. Two failures on a loaded host followed from that. waitForArtifact budgeted by attempt count (maxAttempts = 400), not wall clock. Each attempt costs an HTTP round trip plus a 25 ms sleep, and both stretch under CPU contention, so the budget shrank exactly when validation needed longer. On exhaustion it returned the last response instead of failing, so the large-upload test saw 'validating' where it asserted 'staged'. The second failure followed from the first. On exhaustion the test's finally closed the server and removed the temp dir while validation was still running. Store calls inside that task re-resolve the module-scoped getDb() handle at call time (server/db.ts:278), and starting the next server re-points that variable, so the leaked task wrote its connector_instances row into a later test's database. That is the off-by-one row count, and it is why the victim moved between runs (tests 7, 14 and 19 observed locally; 8 and 15 in the referenced report). Budget by wall clock and throw on exhaustion, so a timeout fails the test that actually timed out. Drain artifacts to a terminal status before teardown, so a slow validation cannot write into the next test's database. Test-only; no production code changes. Under a concurrent full suite on a 24-core host at load average ~200: 6 of 7 runs failed before, 10 of 10 clean after. The underlying concern -- an unowned background task writing through a mutable getDb() singleton that shutdown deliberately does not drain -- is production-shaped and left for a separate change. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI

…timeout-honesty test `run-executor-timeout-honesty.test.ts` was the last load-sensitive test in the suite: it set `maxRunWallClockMs` to 150 ms and expected a real connector subprocess to spawn, complete the START handshake, durably ingest 5 records and emit a PROGRESS `phase_boundary` inside that one window. `maxRunWallClockMs` is a resettable no-progress allowance, not a total-run budget — `markProgress` re-arms the timer on every PROGRESS message — but the watchdog is armed at construction, before the subprocess is spawned. With no PROGRESS before the phase boundary, the first allowance had to cover `node` startup, the handshake and the ingest round-trip together. Measured via the run-executor's own `elapsed_ms` at the phase-boundary latch (24-core host): 84-124 ms idle, up to 148 ms under load, against a 150 ms allowance. A 2 ms margin. That metric spans the whole attempt from `startedAt`, so it is the right diagnostic for what the old allowance had to cover in one stretch — but it is NOT a measure of this change's effect. This raises the no-progress allowance to 400 ms and resets it on connector readiness; the completion case stays at three allowances and the secondary ceiling at four. Startup remains subject to the initial allowance. - Both generated connectors emit a plain `PROGRESS` "ready" message immediately after START, so the ingest round-trip no longer shares one allowance with process startup. This does not make startup unbounded and does not remove timing sensitivity. Verified by counting `markProgress` calls across the 8 tests: 39 re-arms with the message versus 31 without — exactly 8 more, one per connector spawn. (`elapsed_ms` cannot show this: `startedAt` is stamped before the watchdog exists, so the re-arm does not move it.) - The allowance and the post-boundary sleep become named constants (`WALL_CLOCK_BUDGET_MS`, `POST_BOUNDARY_SLEEP_MS = 3x`), so the invariant ("the sleep must out-run the allowance") is stated rather than encoded in two unrelated literals. Timeout honesty is unweakened. Every "must time out" test still hangs forever with no further progress, so only the watchdog can end those runs. The phase-boundary test's sleep is still a multiple of the allowance, so a re-armed (rather than disarmed) watchdog would still fail it. The control test that asserts a plain PROGRESS does not disarm the watchdog now exercises that distinction instead of assuming it, because its connector emits one. No headroom multiplier or reduction in flake frequency is claimed; passing runs do not establish a causal rate change, and reliability under contention is unproved. Production code is untouched. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

…ble policy The gate ran at most 2 test files at once on every host and profile. The number lived inline in run-tests.ts as a single expression, so the only way to check it was to read it, and the only test that covered it matched the runner's source text. Extract the decision into scripts/file-concurrency.ts as a function over an explicit input record, and raise the memory-default cap to 8. PostgreSQL stays at 2: its restore target is not per-file allocated, so files sharing that lane contend on a single resource, and a cap alone is not evidence that sharing it more widely is safe. The cap is an upper bound, not a target. It is clamped by available CPUs and by the number of files selected, so a 4-CPU hosted runner resolves to 4 and never reaches 8. A positive PDPP_TEST_CONCURRENCY override still wins and is deliberately left unclamped. Replace the source-text assertions with cases that call the policy and check the worker count it returns, plus one control that the runner still resolves through the policy rather than its own inline default. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

composed-origin.test.ts needs the console's production build and builds it itself when it is absent. That build runs in a subprocess whose output the test captures rather than forwards, and Node's test runner owns the test file's stdio, so nothing the file writes while the build runs reaches the gate. The gate kills a file that goes 120 seconds without output, so a cold console build is indistinguishable from a hung file for as long as it takes. How long it takes depends on the share of the machine the build gets, which is what makes this surface now: running more test files at once gives the build less. Measured here with the console build cold, the file's longest silence was 31.0s of a 33.5s run on a 24-core machine. A hosted runner has four CPUs and slower ones, and it ran three other test files alongside this one, which was enough to cross 120 seconds and have the file killed. Building the console alongside the other build-time workspace prerequisites removes the silence rather than widening the budget around it: the same measurement with the console already built is a 2.6s longest silence in a 3.6s run, against the unchanged 120s budget. The build also gets its own named step, so when it breaks it reads as a build failure instead of an unexplained timeout in an unrelated test. The accompanying test asserts the workflow builds the console before it runs the gate, so dropping the step fails a test rather than returning as an intermittent timeout blamed on concurrency. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

… policy Three statements in the policy module did not match what the code does. The module claimed the clamps meant raising the ceiling "cannot change what CI actually runs" on a small machine. That is false. Running the old expression and the new policy side by side for a 4-CPU host with more files than workers gives 2 before and 4 after. A 4-CPU host does not reach 8, but it does change. The clamps bound how far the raise can go; they do not show the result is safe, and the comment now says which of those two it is. The module described a non-integer override as ignored. The override reaches this function through Number.parseInt in the runner, which truncates instead of rejecting, so PDPP_TEST_CONCURRENCY=1.5 arrives as 1 and is honoured. Executed: "1.5" resolves to 1 worker and "32junk" to 32, while "0", "-1" and "abc" fall back to the profile default. The comment now separates what the policy validates from what the environment parser already accepted, and says that tightening the parser would be its own change. The module cited docs/gate-concurrency.md as holding the measurement behind the number. On the default branch that document names 6 as its measured ceiling and advises against raising the cap, so a reader who follows the citation today lands on a document that contradicts the code. The comment now says so and names the rewrite that supersedes it as a merge-order dependency rather than a tidiness preference. A fourth comment claimed `|| 1` rather than `?? 1` was load-bearing for the zero-file case. The Math.max(1, ...) floor already guarantees that and the two operators are interchangeable here, so the comment no longer claims an invariant it does not carry. Comments only: no executable line changes, and the policy tests still pass 10/10. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

The policy file carried 67 comment lines for 21 lines of code. The tests carry the cases; the header now states only the invariant. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

…ld guard The trimmed policy header said a non-positive or NaN override "falls back to the profile cap". It falls back to the profile default, which the same expression then clamps by CPUs and file count: 4 CPUs with an override of 0 resolves to 4, not 8, and 24 CPUs with 3 files resolves to 3. Reworded to say so. The same header cited docs/gate-concurrency.md, which does not exist at that path. The file is reference-implementation/docs/gate-concurrency.md. The Build console workflow step carried ten comment lines wrapping four executable ones, restating a commit message where nobody debugging a red build will read it. Cut to one line and a pointer to the test that enforces the step. The second case in ci-console-prebuild.test.ts asserted only that apps/console defines some build script, which is true whatever the workflow builds. It now reads the build command out of composed-origin.test.ts's own spawn arguments and asserts the workflow runs that exact command, so the prebuild and the fallback it exists to prevent cannot drift apart. Comments and tests only; no change to the policy or the workflow's behaviour. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

ingestRecord acknowledges before its index maintenance finishes. The work is scheduled onto a per-connector-instance lane (connectorInstanceIndexTails in server/records.ts) that the promise ingestRecord returns does not cover, so a teardown awaiting only the caller-visible promises can close the database under work it started. The lane then fails with "[db] No database is open" from a promise nobody awaits, which Node reports as an unhandled rejection against whichever test is running at the time. Reproduced deterministically rather than by losing the race: ingesting a record and calling closeDb() without draining logs "deferred index maintenance failed ... [db] No database is open", the exact message from the failing continuous-integration run; adding the drain between them removes it. Whether the race is lost is a timing question, which is why these tests pass on an idle machine and failed when the gate began running more files at once. server/records.ts already exports drainConnectorInstanceIndexWorkForTests for this, and two other test files already call it before their teardown. This adds it to the five SQLite teardowns in connector-instance-writer-paths.test.ts that were missing it. The Postgres case tears down through closePostgresStorage and does not use the SQLite lane. The accompanying test asserts the drain-before-close ordering and that every SQLite teardown in that file keeps it, since the failure it prevents is invisible on an unloaded machine and nothing else would catch its removal. No production code changes, and no change to the concurrency policy. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

The policy module cites this document as the reasoning behind its cap, but the document described the cap it replaced. It said file concurrency was 2 by default, that 8 was an override and "not the effective default on any host", and it advised against changing the default at all. A reader following the citation to check the 8 was told the 8 does not exist. It also named four tests that failed only at high concurrency, called them contention artifacts rather than code defects, and used them as the reason to stay slow. That reading was wrong, and this branch is where it was disproved: the three SQLite writer-path tests were closing the database while deferred index maintenance they had started was still running, and the upload test let a detached validation task outlive it. All four were real defects and are repaired at the root here. The document now says so, and no longer lists the tests by title, since the titles were only ever a symptom list. Rewritten to state the current policy: caps of 8 for the memory profile and 2 for PostgreSQL, clamped by CPU count and selected file count, floored at 1, with a positive override left unclamped. The wall-clock pair behind the number is kept along with its own limits, including that both runs failed identically and so establish nothing about safety on their own. Structured to match the rewrite in #90, which trims the archive references from the same file: shared headings, and the sections that PR owns are left byte-identical, so the rebase touches only the lines changed here. Documentation only. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

Every case in this file holds a connector-instance write gate on purpose while a second writer queues behind it, then performs real durable work before releasing. The queued writer waits under connectorInstanceLockWaitMs(), a real setTimeout whose production default is 2000ms. That made the assertions a host-speed race: measured here, the first case's held window is 531-585ms on an idle 24-core host, ~3.4x under the budget, and a hosted 4-CPU runner at the gate's concurrency cap is routinely slower than that margin. When the budget expires the queued writer rejects with ConnectorInstanceAdmissionError from a promise not yet awaited (unhandledRejection), and the poisoned teardown closes the database under the next case, which then fails with '[db] No database is open'. That is the exact three-failure signature CI reported. Reproduced by shrinking the budget rather than by slowing the host: at PDPP_INGEST_LOCK_WAIT_MS of 200, 50 and 10 the file fails the same three cases with the same messages in the same order (2 pass / 3 fail). With this change it is 5 pass / 0 fail at all three. Mutation-checked both ways: reverting the change restores 2/3, and breaking the keyed gate's serialization still fails 2 cases, so the coverage is intact. The runner spawns one process per test file (scripts/run-tests.ts:453), so this assignment cannot reach another file. No production code changed, no cap lowered, no retry, no serialization by file name. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

… commands The three writer-path tests had two causes, not one: the undrained index lane and a wall-clock race against the two-second admission budget on slow runners. The override examples used pnpm and omitted the required test profile. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

One pass of edge growth left statements straddling the widened range, so Stryker generated nothing for code the revision changed while the run still reported completion -- the silent false evidence this widening exists to prevent. Real ASTs make the shape ordinary, because statement spans partially overlap. An `if`'s consequent block ends on the line its alternative begins, so a hunk on `} else {` is contained by both blocks; growing to the smallest containing statement returns the consequent and leaves the alternative straddled. On the minimal shape, Stryker 10.0.0 instruments 6 mutants under the one-pass scope 3-5 and 8 under the fixpoint scope 3-7: the two mutants in the else block were lost. The same failure hits a hunk that starts inside a nested statement and runs into the next top-level one -- the start edge grows to the innermost statement it cut and leaves the encloser straddled. Growth now repeats until no statement straddles the range. Termination is by monotonicity: each step only moves edges outward, bounded by the outermost statement, with a step cap as a guard against a malformed boundary set. Two guards keep it tight and make it converge: a containing statement whose span equals the range is not progress and is skipped, and the loop stops once the range is exactly a statement with nothing straddling it -- so a one-line change inside a long function still scopes to its own line rather than following the function out to the module. The comment at the previous single-pass call claimed the edges were "treated repeatedly" and named this exact failure as handled. It now describes what the code does. Three tests, each observed to fail against the previous head and pass here: a `} else {` hunk, the same shape nested one level in, and a hunk starting inside a nested statement. Suite is 102 tests, up from 99; none deleted or weakened. Re-derived PR #83's scope with the fixpoint widening: the six ranges and the 104-mutant instrumentation count are unchanged, so the recorded numbers stand. Closes the round-2 findings N1 (REVIEW-93-R2-0909) and R1 (SIGNOFF-FABLE-93-R2-0909). Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

tnunamak added 10 commits September 9, 2026 23:12

fix(manual-upload): await validation tasks before test teardown

ebc10db

Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI

test(gate): state the concurrency policy in one comment

3c55bd9

The policy file carried 67 comment lines for 21 lines of code. The tests carry the cases; the header now states only the invariant. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>

tnunamak force-pushed the test/file-concurrency-policy branch from e9b1fba to 121eb46 Compare September 10, 2026 04:21

tnunamak added 2 commits September 9, 2026 23:53

tnunamak mentioned this pull request Sep 10, 2026

Scope mutation testing to the changed line ranges #93

Merged

tnunamak merged commit 4e519ef into main Sep 10, 2026
15 of 16 checks passed

tnunamak deleted the test/file-concurrency-policy branch September 10, 2026 14:36

tnunamak mentioned this pull request Sep 10, 2026

perf(test): raise memory-profile file concurrency to eight #50

Closed

Navigation Menu

Uh oh!

Raise memory-default gate file concurrency to 8 behind a testable policy - #83

Raise memory-default gate file concurrency to 8 behind a testable policy#83
tnunamak merged 12 commits into
mainfrom
test/file-concurrency-policy

tnunamak commented Sep 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

tnunamak commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

tnunamak commented Sep 9, 2026 •

edited

Loading