feat: add TRANSACTION_TIMEOUT logical deadline

[apstndb]

Summary

Implements TRANSACTION_TIMEOUT as a logical read/write deadline for #482.

• Duration or NULL; NULL/0 means no additional transaction deadline.
• The duration is captured for the logical owner. Pending SET LOCAL may select it before the first real database RPC; changing it after the budget starts is rejected. Session SET or RESET after BEGIN applies to a later owner.
• The single total budget starts at the first real database RPC, including constructor BeginTransaction on pinned Go Spanner v1.95.0 ReadWriteStmtBasedTransaction. Client-only BEGIN/SHOW and buffering automatic DML do not start it.
• The deadline is preserved across physical reconstruction. SQL, Batch DML, commit, and replay RPCs receive the remaining budget together with the caller and STATEMENT_TIMEOUT deadlines.
• Expiry cancels in-flight RPCs without waiting for the transaction mutex, retires only the matching owner, stops its heartbeat, and restores SET LOCAL at the serialized session safe point (start/end of ExecuteStatement, and Close). Timer goroutines never call Registry.Set.
• Failed constructor keeps pending identity, undo, and an already-armed budget. Idle constructor failure still leaves no owner.
• Distinct from unimplemented user-idle expiry (Expire user-idle transactions independently of keepalive traffic #357). ABORTED retries (Implement bounded ABORTED retries using the SAVEPOINT replay machinery #293) are not implemented and are not added here.

Does not change automatic-DML queue/flush signatures owned by #979/#401.

Fixes #482

Test plan

• Focused unit tests for capture, SET LOCAL vs session SET/RESET, constructor start, failed construction, mutex-held cancel, idle expiry then BEGIN, delayed old-owner callback, SAVEPOINT budget reuse, automatic DML, NULL/0, shorter caller context
• go test -short ./..., golangci-lint run, make fmt-check (exit 0)
• make check-race (exit 0)
• go test -short -coverpkg=./internal/mycli ./internal/mycli coverage 82.6% (floor 80%)
• Hosted CI make test / emulator (this VM has no Docker; local make check failed only on integration container startup)

  

[github-actions]

Code Metrics Report

📊 View detailed coverage report (available for 7 days)

	main (c7ed015)	#980 (97dfd5c)	+/-
Coverage	86.6%	86.7%	+0.1%
Code to Test Ratio	1:2.3	1:2.3	+0.0
Test Execution Time	1m44s	2m11s	+27s

Details

  |                     | main (c7ed015) | #980 (97dfd5c) |  +/-  |
  |---------------------|----------------|----------------|-------|
+ | Coverage            |          86.6% |          86.7% | +0.1% |
  |   Files             |            113 |            115 |    +2 |
  |   Lines             |          10715 |          10951 |  +236 |
+ |   Covered           |           9280 |           9499 |  +219 |
+ | Code to Test Ratio  |          1:2.3 |          1:2.3 |  +0.0 |
  |   Code              |          23964 |          24397 |  +433 |
+ |   Test              |          55774 |          57869 | +2095 |
- | Test Execution Time |          1m44s |          2m11s |  +27s |

Code coverage of files in pull request scope (88.7% → 89.1%, patch 89.8%)

Files	Coverage	+/-	Patch Coverage	Status
internal/mycli/config.go	91.0%	+0.0%	-	affected
internal/mycli/execute_dml.go	90.5%	+0.1%	100.0%	modified
internal/mycli/execute_partitioned.go	90.6%	-0.9%	-	affected
internal/mycli/execute_sql.go	94.6%	+0.4%	-	affected
internal/mycli/savepoint_replay.go	85.1%	+0.1%	100.0%	modified
internal/mycli/session.go	86.4%	+1.2%	100.0%	modified
internal/mycli/session_transaction_context.go	83.3%	-0.9%	75.0%	modified
internal/mycli/statements_system_variable.go	92.1%	-0.2%	86.6%	modified
internal/mycli/statements_transaction.go	73.6%	+0.5%	100.0%	modified
internal/mycli/system_variables.go	96.5%	0.0%	-	modified
internal/mycli/tls_transport.go	94.3%	+94.3%	-	affected
internal/mycli/transaction_manager.go	87.9%	-0.1%	88.9%	modified
internal/mycli/transaction_manager_auto_dml.go	96.5%	+0.0%	100.0%	modified
internal/mycli/transaction_timeout.go	91.1%	+91.1%	89.0%	added
internal/mycli/var_defs.go	86.5%	+0.3%	100.0%	modified

---

Reported by octocov

[cursor]

051c903

[apstndb]

Reviewed HEAD 051c9032c31cea94aeaaee251e271e826cce0a78. Two blocking findings are independently reproduced with the real client connected to the existing local fake gRPC server:

1. [P1] Pass the remaining deadline to every Batch DML RPC. RunInNewOrExistRwTxLocked shortens its own ctx, but its callback still captures the caller's original context, so executeBatchDML sends that original context to BatchUpdateWithOptions. The automatic flush path also sends the unbound context except at one caller. With a 20-second caller and a 2-second transaction, the actual server-side Batch DML context had the 20-second deadline in both manual-batch and FlushAutomaticDML probes. These RPCs hold the transaction mutex, so the expiry watcher cannot retire the owner to compensate. Bind the context at the actual RPC boundary (or explicitly pass the bound context into the callback) for every route, including RUN BATCH and flush-before-read. Add actual fake-RPC deadline assertions and an in-flight cancellation/owner-retirement regression; merely checking the stored owner deadline or a COMMIT flush does not cover these paths. Locations: callback dispatch, manual Batch DML, automatic flush.

2. [P2] Freeze the selected duration after first database use even when it is unlimited. The freeze check uses a nonzero deadline, but NULL/0 never sets one. Starting a transaction with the default NULL, observing its constructor BeginTransaction RPC, and executing SET LOCAL TRANSACTION_TIMEOUT = '1h' succeeds instead of returning the frozen-owner error. A later operation can therefore start a new budget on an already active owner, contrary to the pending-only selection contract. Track first-use/activation independently of whether a timer exists, preserve it across failed construction/replacement as specified, and cover both NULL and zero. Location: freeze and arming.

The independent probes failed with those exact causes on both 586684b and the current merged HEAD. Review remains open while these are fixed.

The required pre-push gate also remains unmet: the receipt says make check exited 2 because Docker was absent, followed by another push. Prepare a working Docker/emulator environment or reuse the already demonstrated full-validation route, then run the full make check successfully before every further push, plus make check-race for these lifecycle fixes. Capture commands, exits and exact source identity. Short tests/lint and later hosted CI are not a substitute for that repository requirement. If environment recovery truly blocks the lane after concrete attempts, report the evidence before publishing again. Preserve all published history and fix additively; no reset, rebase or force-push is authorized.

[apstndb]

Additional blocking finding on the same reviewed HEAD 051c9032c31cea94aeaaee251e271e826cce0a78:

[P1] Make expiry retirement and the statement-entry restoration boundary atomic. executeStatement checks/drains LOCAL restoration only once at entry. If the timeout callback retires owner A immediately after that check, the statement can create owner B before A's detached undo is drained. The end-of-statement check then sees B and skips restoration, so B inherits A's expired LOCAL settings.

I reproduced this deterministically with an overlay hook immediately after the new entry drain, using the existing expiry callback before the actual BEGIN handler. A had session timeout 1h, LOCAL timeout 2h and LOCAL CLI_VERBOSE=TRUE; after A's expiry and B's BEGIN, verbose was still TRUE and B captured 2h rather than the restored 1h. This is a forced callback interleaving, not a wall-clock timing claim. No production behavior was changed by the probe beyond scheduling that callback at the existing race window.

Please implement an atomic handoff under the lifecycle synchronization so an expired owner's undo is restored before another statement reads defaults or installs a new owner. Registry setters must remain outside timer goroutines, and an unsynchronized/standalone execution flag or another one-shot drain would leave the same race. Add this exact ordering regression alongside expiry-before-entry and in-flight cancellation cases. Location: entry restoration, timer retirement.

Keep the same additive PR and the previously requested full make check plus race gate before publishing the fixes.

[apstndb]

6e3308e

[apstndb]

Review update for exact HEAD 6e3308e1f59a7789ae891a102b586ba5f856df6e:

The independent actual-RPC Batch DML deadline probes and NULL/0 first-use rejection now pass. The original timeout/verbose owner-install regression also passes. Thank you for keeping the published history additive.

[P1] The expiry/LOCAL restoration boundary is still incomplete. I reproduced both remaining cases with the new afterEntryRestore hook calling watchTransactionDeadline(ownerA, expiredContext()), then the actual statement handler:

• Session RPC_PRIORITY=LOW, A has SET LOCAL RPC_PRIORITY=HIGH; expire A after entry restoration, then execute BEGIN. B captures HIGH although the registry is restored to LOW. BeginPendingTransaction resolves priority/isolation before withOwnerInstallAfterRestore restores the registry (lines 702 onward).
• Session timeout1h, A LOCAL timeout2h; expire A at the same boundary, then execute ordinary SET TRANSACTION_TIMEOUT = '3h'. SET reports success, but the post-statement drain overwrites it with1h. Guarding only owner constructors cannot protect successful session setters or statement defaults read before owner installation (statement entry/defaults).

These are deterministic forced-interleaving reproductions on this HEAD, not elapsed-time claims. Please fix the shared statement/retirement protocol, cover ordinary SET plus frozen priority/isolation and statement-default reads, and retain constructor/in-flight cancellation and nested execution coverage. Do not hold a non-reentrant statement mutex across an RPC or move Registry.Set into timer callbacks. A synchronized, owner-aware deferred-retirement protocol is one possible design; choose the smallest protocol with a clear ordering argument. Moving two default reads alone leaves the successful SET regression.

Validation remains full make check before every further push and make check-race, with full source identity and whether Docker integration actually ran. Please return the relevant successful gate summary, since the last progress text questioned unit-only execution. Current hosted CI is green; this source/runtime finding still blocks acceptance. Preserve published history and use additive fixes only.

[apstndb]

8cd56b7

[apstndb]

Review update for exact HEAD 8cd56b7be9b2710059b38e2a79790b62eb4c7ca8:

All earlier independent probes now pass, including the actual Batch DML deadlines, NULL/0 freeze, expiry before BEGIN with LOCAL priority, and ordinary session SET preservation. The reported uncached full/Docker gate receipt also resolves the earlier unit-only uncertainty.

[P2] Close the final statement-depth/expiry handoff atomically. leaveStatement first calls syncExpiredOwnerRestore, then separately locks and decrements depth. If the timeout callback runs between those operations, it sees depth1, sets expirePending, stops its watcher and leaves undo attached. leaveStatement then changes depth to0 and returns without another retirement. The now-idle owner stays live/expire-pending and retains LOCAL values and queued work until a later command, although its only expiry watcher has already exited.

I reproduced this on this HEAD with one test-only overlay callback immediately after the final syncExpiredOwnerRestore in leaveStatement, before the depth decrement. It invokes the existing expiry callback for A while an actual SHOW VARIABLE CLI_VERBOSE finishes. After return: depth0, non-nil owner with expirePending=true, CLI_VERBOSE still TRUE rather than restored FALSE. This is a forced legal callback ordering; there is no production state mutation in the probe beyond invoking that callback at the existing gap.

Please make the final frame-depth transition and decision to retire an expired matching owner a single tm.mu critical section, then restore detached registry entries outside the lock. Preserve nested-frame semantics and the rule that timer callbacks do not touch the registry. The invariant at that boundary should rule out depth0 with an already-processed expiry still pending and no watcher left to retire it. Add the exact final-frame interleaving regression; another unsynchronized one-shot drain alone simply moves this gap. Location: leaveStatement.

This is a small completion of the new protocol; keep it additive and run full make check plus make check-race before pushing. No merge or history rewrite by the worker.

[apstndb]

8d375df

[apstndb]

Independent re-review of exact HEAD 8d375df628ec20418cbfe96189aea642be71970a: no remaining actionable findings in the reviewed scope.

The earlier findings are resolved additively: 9f05be0/6e3308e bind the actual manual/automatic/replay Batch DML RPCs and track first use independently of NULL/zero; 8cd56b7 preserves expired LOCAL restoration across BEGIN/default capture and ordinary SET; 8d375df makes final-frame depth/retirement atomic while keeping registry restoration outside the transaction mutex and timer callbacks. Source review also covers owner identity, nested statement frames, reconstruction and cleanup ordering.

On this exact HEAD, my independent fake-gRPC/deadline/forced-interleaving probes and the timeout test suite pass, including every previously failing reproduction. Independent full make check passed locally with Docker-backed integration, lint and format checks. Current hosted race and all other applicable CI checks pass; required-check wait passed, the branch is mergeable, and no unresolved inline or review-level feedback remains. The implementation worker also supplied successful full/race gate receipts for this source.

This acceptance concerns the logical read/write timeout and its integration; it does not add user-idle expiry or automatic ABORTED retry, and local fake/emulator tests are not a claim of managed-service validation.