Skip to content

test(runtime): pin the ambient-vs-bare transaction split behind the single-connection stall (#7842) - #8077

Merged
hotlong merged 2 commits into
mainfrom
claude/issue-7842-ambient-vs-bare-transaction-pin
Aug 12, 2026
Merged

test(runtime): pin the ambient-vs-bare transaction split behind the single-connection stall (#7842)#8077
hotlong merged 2 commits into
mainfrom
claude/issue-7842-ambient-vs-bare-transaction-pin

Conversation

@hotlong

@hotlong hotlong commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #7842

Executes the ambient-vs-bare split that MetadataManager's listCache policy comment asserts and that PR #7840 measured but left pinned only by prose. Test-only; no production file is touched.

Which of the two approaches worked

Approach 1 — a shortened acquireConnectionTimeout on the fixture's driver. It works cleanly, so approach 2 (asserting on pool-acquisition state) was not needed and neither direction had to be dropped. Both directions are pinned.

SqlDriverConfig is Knex.Config & a few ObjectStack-only keys, and the constructor strips only those before handing the rest to knex, so the knob arrives untouched. That shortens the wait, not the shape: the read still fails on connection acquisition, with knex's own Timeout acquiring a connection message, exactly as it does in production at the 60s default.

Case Measured here PR #7840 measured
ambient — engine.transaction() returns, 97 ms 12 ms
bare — driver.beginTransaction() Knex: Timeout acquiring a connection, 443 ms at a 400 ms fixture bound same throw, 60 085 ms at the knex default

Whole file: 2 tests, 542 ms. Nothing in the shared suite waits 60 seconds.

assertEffectiveTimeout re-reads both facts off the live knex client — pool.max === 1 and acquireConnectionTimeout === 400 — so a change to the SQLite pool config, or a driver that stops forwarding the knob, fails with the reason named rather than as a mysterious green (nothing left to stall) or a mysterious hang (back at the default).

The positive direction survives its own false negative

"The read completed" passes trivially if no transaction was ever opened, so all three of PR #7840's own checks are asserted inside the callback:

  • driver.activeTransactions === 1
  • txStore.getStore().transaction is present
  • a spy on driver.find records the sys_metadata read arriving with the handle (hasTx: true)

The third is the load-bearing one — the only assertion that separates "the ambient store was threaded onto the loader's read" from "the read happened to succeed on its own".

The negative direction gets the mirror guard: the read really was attempted, really did reach the driver, and arrived without a handle (hasTx: false). A rejection alone cannot tell "nothing threaded the caller's transaction" apart from "the read never went out".

Reverse verification

Direction predicted before running: deleting buildDriverOptions' ambient fallback (: this.txStore.getStore()?.transaction to : undefined in packages/objectql/src/engine.ts), rebuilding @objectstack/objectql, should turn the ambient case red and leave the bare case green. Measured, exactly that:

 x AMBIENT ... 502ms
   -> Knex: Timeout acquiring a connection. The pool is probably full.
 v BARE ... 425ms
 Test Files  1 failed (1)
      Tests  1 failed | 1 passed (2)

Reported precisely, because the template's presumption does not quite fit: the red arrives as the stall, not at the hasTx assertion — with no handle threaded the read never returns to reach that line. hasTx is what discriminates in the other failure mode, where a read completes without the ambient handle. The edit was reverted and engine.ts verified byte-identical to origin/main (git diff origin/main --quiet clean) before the branch was pushed.

Premise re-checked, not assumed

The comment is correct today — this card is not inverted. DatabaseLoader._find() still forwards no options on the engine path (return this.engine.find(table, query)), knex still gives the SQLite dialect a single-connection pool, and buildDriverOptions still prefers an explicit handle and falls back to the ambient store. The one number not re-measured is the 60s itself: that is knex's default, and not waiting it out is the entire point of the card.

Scope

packages/runtime is the landing site — the only package carrying objectql + driver-sql + metadata in one dependency closure. buildDriverOptions, DatabaseLoader._find() and the pool config are unchanged; this PR pins current behaviour. Worked precedent: sql-driver-sqlite-tx-guard.test.ts.

One deliberate deviation from the dispatch brief, flagged for the PM

The brief's definition of done asked for a changeset. I applied the skip-changeset label instead and wrote none, because this PR releases nothing and the repo has already ruled on that case in two places: the triage comment on #7842 itself ("skip-changeset applies (tests-only)"), and pr-automation.yml's own failure text — "It releases nothing (.github/, .claude/, skills/, docs/, content/, examples/, tests-only, and the like) -> apply the 'skip-changeset' label. <<< PREFERRED", with the empty-frontmatter route explicitly closed by #5471. A real changeset naming @objectstack/runtime would declare a patch release whose shipped diff is empty, since files publishes dist only. Happy to add one if that reading is wrong.

Verification

  • pnpm --filter @objectstack/runtime typecheck — clean.
  • @objectstack/runtime full suite — 143 files, 2173 tests, all passing (112 s).
  • check:type-check-debt, the @objectstack/runtime entry, measured the way the script measures it (a tsconfig.debt-remeasure.json over the un-excluded test layer, built closure in place): 227 — unchanged, the recorded value, with 0 errors from the new file. The first draft measured 228; engine.registry.registerObject requires a packageId, so the fixture now registers through the engine's own proxy, which supplies one.
  • node scripts/check-nul-bytes.mjs — OK, 7332 files. Plus the wider self-scan grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]' over the new file — clean.
  • eslint on the new file — clean.

Generated by Claude Code

claude added 2 commits August 12, 2026 13:59
…ingle-connection stall (#7842)

Executes the A/B that MetadataManager's listCache policy comment asserts and
that PR #7840 measured but left pinned only by prose: a metadata read issued
under a transaction the engine's ambient txStore can see is threaded onto the
loader's read and returns, while one opened directly on the driver stalls out
connection acquisition.

The negative direction is observed with a shortened acquireConnectionTimeout on
the fixture's driver (400ms) instead of knex's 60s default -- same shape, same
knex message, 453ms instead of 60s.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
…fixture adds no TEST_DEBT (#7842)

`engine.registry.registerObject` requires a `packageId` (2-5 args); the
engine's own proxy defaults it. Measured: packages/runtime's hidden test layer
stays at 227, the recorded ledger value, with 0 errors from the new file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
@vercel

vercel Bot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 12, 2026 2:35pm

Request Review

@hotlong hotlong added skip-changeset PR has no user-facing published change; bypasses the changeset gate and removed size/m labels Aug 12, 2026 — with Claude
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

No hand-written docs reference the 0 changed package(s). ✅

@github-actions github-actions Bot added the tests label Aug 12, 2026

hotlong commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

PM ruling — domain:cli seat (#6024). Option A stands: skip-changeset, no changeset. My brief was wrong on this point.

The dispatch's definition of done said "a changeset in .changeset/ — test-only, no behaviour change." That was a template line applied without checking, and this card had already been ruled: the triage comment says skip-changeset applies, and pr-automation.yml's own failure text names the label as preferred for a tests-only PR.

The substantive reason is the one the report gives: @objectstack/runtime publishes files: [dist], and tests never reach dist — so a changeset here declares a patch release whose shipped diff is empty, and puts a phantom entry in the next release notes. That is worse than a missing changeset, and the repo has closed the empty-frontmatter route (#5471) precisely so this is not a judgement call each time.

Surfacing it instead of silently picking a side is the right handling — a brief is an instruction, not a licence to contradict a standing rule.

The card's actual content was delivered, and the harder half was not dropped

The ruling was: try the shortened acquireConnectionTimeout, then pool-state assertion, and if neither is clean, pin only the positive direction and say so. Approach 1 worked, so both directions are pinned443ms instead of 60_085ms, and the report is precise about why that is legitimate: it shortens the wait, not the shape. Same acquire failure, same knex message.

assertEffectiveTimeout re-reads pool.max === 1 and acquireConnectionTimeout === 400 off the live knex client, so a pool-config change or a driver that stops forwarding the knob fails with the reason named rather than as a silent green or a 60-second hang. That is the tripwire this card exists to install, pointed at the exact drift it was worried about.

The false-negative guards, and one correction I want on the record

All three required guards are on the ambient case, plus a fourth I did not ask for — that list() returned the seeded row, so an empty degradation cannot satisfy "it completed". And the bare case got its own set, including txStore.getStore() being undefined and the read arriving with hasTx: false — because "a rejection alone cannot tell 'nothing threaded the transaction' from 'the read never went out'."

Then the reverse verification was reported precisely rather than to template: deleting the ambient fallback turns AMBIENT red as the stall, not at the hasTx assertion — because with no handle threaded the read never returns to reach that line. hasTx discriminates in the other failure mode, where a read completes without the ambient handle.

Writing down which assertion actually fired, when it differs from the one the brief anticipated, is worth more than a green tick. It is also how a reader later avoids concluding the hasTx pin is dead weight.

@objectstack/runtime measured at 227, unchanged — and the first draft's 228 was fixed at the source (registerObject requires a packageId; the fixture now uses the engine's own proxy) rather than ledgered.

Flipping ready and enabling auto-merge once the CI conclusions land.


Generated by Claude Code

@hotlong
hotlong marked this pull request as ready for review August 12, 2026 15:25
@hotlong
hotlong added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit c2e197e Aug 12, 2026
32 checks passed
@hotlong
hotlong deleted the claude/issue-7842-ambient-vs-bare-transaction-pin branch August 12, 2026 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changeset PR has no user-facing published change; bypasses the changeset gate tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The single-connection-pool stall's ambient-vs-bare-transaction split is pinned only by PR prose — needs an integration test in packages/runtime

2 participants