Skip to content

docs(driver-sql): pool docblocks name the factory's max 5, not knex's max 10 - #14773

Draft
os-musk wants to merge 3 commits into
mainfrom
claude/issue-14588-driver-sql-pool-docblocks
Draft

docs(driver-sql): pool docblocks name the factory's max 5, not knex's max 10#14773
os-musk wants to merge 3 commits into
mainfrom
claude/issue-14588-driver-sql-pool-docblocks

Conversation

@os-musk

@os-musk os-musk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #14588

Two docblocks in packages/drivers/driver-sql/src/sql-driver.ts justified themselves with knex's own poolDefaults(){min: 2, max: 10} — as if that were the pool size the platform runs at. It is not. Every SQL datasource the platform composes goes through buildSqlPool in service-datasource's default-datasource-driver-factory.ts, which hands it an explicit {min: 0, max: 5} unless the datasource declares its own pool, so nothing the factory builds ever reaches knex's default.

Both docblocks now name max: 5 and where that number lives, and keep the knex fact as one clause — it is true, and it is precisely why the misreading is easy to have. Naming the factory file is the point: a reader who learns only "max 5" goes looking in the wrong package next time.

Comment-only. No behaviour change, no executable line moves.

The two passages

ensureSequencesTable (the MySQL DDL branch), before:

there the roomy pool (max=10) lets a fresh connection create the table safely.

after:

there the pool has room (max=5) for a fresh connection to create the table safely. The 5 is buildSqlPool's — service-datasource's default-datasource-driver-factory.ts gives every datasource it composes {min:0,max:5} unless one declares its own pool — not knex's roomier poolDefaults() of {min:2,max:10}, which the factory never reaches.

assertBareKnexSafe (the SQLite dead-lock guard), before:

No-op in production (zero overhead on the hot path) and on every non-SQLite dialect, whose roomy pools (max at least 10) cannot exhibit the single-connection dead-lock.

after:

No-op in production (zero overhead on the hot path) and on every non-SQLite dialect, whose pool holds more than one connection and so cannot exhibit the single-connection dead-lock. That headroom is max: 5, from buildSqlPool in service-datasource's default-datasource-driver-factory.ts, which gives every datasource it composes {min:0,max:5} unless one declares its own pool — not knex's roomier poolDefaults() of {min:2,max:10}, which the factory never reaches.

Measured on this branch, not inherited

Every prior reading of this card was stale by the time it was used, so all of it was re-taken at 224f8ea4a (the merge base).

Closed population. grep -n "roomy" over sql-driver.ts returns exactly two sites — :5901 and :8204 — so the population is two, not a sample. Widened across packages/drivers/driver-sql/** for the claim rather than the word (roomy, max=10, max: 10, poolDefaults, min: 2), the only other hits are README.md examples where an author explicitly declares pool: { min: 2, max: 10 } in SqlDriverConfig. A declared pool is honoured verbatim, so those are valid config, not a restatement of the stale default. Outside the declared write surface and not touched — reported to triage.

The factory default, verified rather than inherited. buildSqlPool at packages/services/service-datasource/src/default-datasource-driver-factory.ts:626 reads min: typeof pool.min === 'number' ? pool.min : 0 / max: typeof pool.max === 'number' ? pool.max : 5, and is wired into both the postgres (:1077) and mysql (:1134) branches. The sqlite branch passes no pool at all — SQLite's max: 1 comes from knex's own sqlite3 dialect override, defaults({min: 1, max: 1}, super.poolDefaults()). Read-only; this PR does not touch that package.

End-to-end composed config, no live database. Driving the real factory:

factory.create({ driver: 'postgres', config: { url: … } })
  knex config.pool = {"min":0,"max":5,"createTimeoutMillis":15000}   tarn live min/max = 0/5
factory.create({ driver: 'mysql',    config: { url: … } })
  knex config.pool = {"min":0,"max":5,"createTimeoutMillis":15000}   tarn live min/max = 0/5
factory.create({ driver: 'postgres', config: { url: … }, pool: { min: 3, max: 9 } })
  knex config.pool = {"min":3,"max":9,"createTimeoutMillis":15000}   tarn live min/max = 3/9

Reproducing the filer's reading exactly. The knex fact holds too: knex 3.3.0 Client.poolDefaults() at lib/client.js:229-231 returns { min: 2, max: 10, propagateCreateError: true }.

Verification — all at 97c42b50d

Comment-only, proven mechanically rather than asserted. Both file versions were emitted through ts.transpileModule with removeComments: true (typescript 6.0.3) and the outputs compared:

emitted bytes before/after: 282535 / 282535
sha256 before: 6acfc0527bfbc52716afffdb7d16e3d28997ecc501eb72a7741de8d726cc5ab3
sha256 after : 6acfc0527bfbc52716afffdb7d16e3d28997ecc501eb72a7741de8d726cc5ab3
IDENTICAL

Tests, both sides. pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2, run at the fix and again with the pre-fix file restored on disk (mutation confirmed by blob hash before the run; restore confirmed byte-identical to HEAD after it):

before: Test Files 152 passed | 9 skipped (161)   Tests 2294 passed | 140 skipped (2434)
after : Test Files 152 passed | 9 skipped (161)   Tests 2294 passed | 140 skipped (2434)

pnpm --filter @objectstack/driver-sql typecheck — exit 0.

Gates. 22 families derived from the actual changeset with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (21 by path, 1 by change kind). All 22 run: 20 exit 0, and 2 land on their own NOT-MEASURED branch, recorded in the gate's own words rather than as green:

  • node scripts/check-test-completeness.mjs — exit 3, no turbo run test log to hand it: "the local reading for this gate is NOT MEASURED … it is not a red, and there is nothing here to fix."
  • pnpm check:dual-build-cjs-loads — exit 3, PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/ … This is NOT a pass: nothing was measured. It needs a full-repo build; CI does that. The byte-identical emit above is the standing reason this diff cannot move any package's built output.

ESLint — a declared narrowing, not a skipped run. Targeted at the one changed file: eslint --no-inline-config --format json reports 1 file linted, 0 errors, 0 warnings. The three pieces the narrowing owes: ① the receiving population is 5768 of 8059 tracked files, computed through eslint's own ESLint.isPathIgnored() against eslint.config.mjs, not guessed; ② the file count is read from the --format json output array; ③ eslint.config.mjs states in its own comment that this repo "runs one eslint.config.mjs, which never enables type-aware linting (no parserOptions.project, no typed @typescript-eslint rules) for ANY file, test or not" — measured there with a positive control — so no untouched file's verdict can depend on this diff. The full repo sweep is CI's run.

Scope held

No changeset — the diff is comment-only and publishes nothing from any released package (the emitted bytes are identical), so skip-changeset applies, per the landed precedent for a comment-only diff.

🤖 Generated with Claude Code

Generated by Claude Code


Generated by Claude Code

… max 10

Two docblocks in `sql-driver.ts` justified themselves with knex's own
`poolDefaults()` (`{min: 2, max: 10}`) as if it were the size the platform
runs at. It is not: `buildSqlPool` in service-datasource's
`default-datasource-driver-factory.ts` hands every SQL datasource an explicit
`{min: 0, max: 5}` unless the datasource declares its own `pool`, so no
factory-composed datasource ever reaches knex's default.

Both docblocks now name `max: 5` and where that number lives, and keep the
knex fact as one clause -- it is true, and it is why the misreading is easy
to have. Comment-only; no executable line moves.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68
@github-actions github-actions Bot added the size/s label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 2 documentable anchor(s).

7 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx (via SqlDriver (symbol, a top-level class))
  • content/docs/data-modeling/index.mdx (via SqlDriver (symbol, a top-level class))
  • content/docs/permissions/tenant-audit-census.mdx (via SqlDriver (symbol, a top-level class))
  • content/docs/plugins/packages.mdx (via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/index.mdx (via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/kernel/lifecycle.mdx (via SqlDriver (symbol, a top-level class))
  • content/docs/protocol/objectql/query-syntax.mdx (via SqlDriver (symbol, a top-level class))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17.mdx (via SqlDriver (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 10 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 7251bad7d8c3a96725b85104b4c7ff73023bdd5bpackageMentionDocs.

Which tree this was computed on

This run read content/docs from a81a46fa1e2625bdb05519158d96579d162c86ae — the merge of head 9921a0cc90060ff0f356fcdbc5f434c5954096ac into base 7251bad7d8c3a96725b85104b4c7ff73023bdd5b, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin a81a46fa1e2625bdb05519158d96579d162c86ae && git checkout a81a46fa1e2625bdb05519158d96579d162c86ae
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7251bad7d8c3a96725b85104b4c7ff73023bdd5b 9921a0cc90060ff0f356fcdbc5f434c5954096ac && git checkout -B drift-repro 7251bad7d8c3a96725b85104b4c7ff73023bdd5b && git merge --no-ff 9921a0cc90060ff0f356fcdbc5f434c5954096ac

node scripts/docs-audit/affected-docs.mjs --json 7251bad7d8c3a96725b85104b4c7ff73023bdd5b

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 7251bad7d8c3a96725b85104b4c7ff73023bdd5b → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-musk os-musk added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Sep 3, 2026 — with Claude

os-musk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

⛔ Held — red on the #14648 queue flake, not on this diff

Test Core (1/6) failed at 01:10:10Z after 22 minutes. Diagnosed from the job log, ⛔ not assumed from the shard name:

FAIL  integration  test/run-dev-unbuilt-workspace.e2e.test.ts
      > the mirror direction: a reader that is never coming back
      > gives up and exits instead of waiting forever
AssertionError: expected 'SIGKILL' to be null
  ❯ test/run-dev-unbuilt-workspace.e2e.test.ts:317:27
Test Files  1 failed | 234 passed (235)

That is #14648, verbatim — same file, same case, same assertion, same line.

And the durations corroborate the standing diagnosis rather than merely matching a name: Duration 951.46s wall against tests 2416.00s — the suite's own test time is 2.5× the wall clock, i.e. heavy oversubscription. UNREAD_HARD_CAP_MS = 40_000 plus child.kill('SIGKILL') mean signal === 'SIGKILL' reports only that the harness's own 40s cap beat the child. It is a wall-clock claim wearing an AssertionError. The same shape was measured on PR #14649 (843.94s wall vs 2151.97s tests).

Why this failure cannot be this PR's

The diff is one file, comment-only, and the dev proved that mechanically rather than asserting it: both versions emitted through ts.transpileModule with removeComments: true produce identical output — 282535 bytes each, sha256 6acfc0527bfbc52716afffdb7d16e3d28997ecc501eb72a7741de8d726cc5ab3 on both sides.

⇒ A behaviour change is impossible here, so a product regression is not among the available explanations. Of the three classes a red belongs to — load, product regression, or a broken validity premise — this one is excluded from the second by construction and lands in the first.

Disposition, per the standing #14648 rule

Card #14588pm:blocked, Blocked-by: #14648, Unlock-action: re-check PR #14773.

One stale reading, recorded so nobody chases it

Check Changeset also shows a failure (00:48:56Z) — superseded. The skip-changeset label landed at ~00:49:25Z and the label-triggered re-run is skipped. ⚠️ Worth knowing generally: a seat's label edit on a PR re-triggers Check Changeset, so an earlier failing run can sit in the check list next to a later correct one. Read the latest run per check name, not the first one you meet.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/s skip-changeset PR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants