Skip to content

Implement RESET ALL with startup snapshot prepare/commit - #976

Merged
apstndb merged 5 commits into
mainfrom
cursor/reset-all-startup-484
Sep 14, 2026
Merged

apstndb merged 5 commits into
mainfrom
cursor/reset-all-startup-484

Conversation

@apstndb

@apstndb apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Fixes #484

RESET ALL restores resettable session variables to the values captured after defaults, config, flags, and --set, and before both --init-command forms. Init-command SQL stays ordinary SQL and can be reset.

Behavior

  • Shared prepare/commit primitive plus Reset(name) / ResetAll core. Single-variable RESET <name> parsing stays in Support RESET of a single session variable using startup values #960.
  • Prepare validates every changed supported value without mutating the live registry or resource graph.
  • Equal-value persistent RESET still retires only the targeted LOCAL undo after whole-operation success.
  • A rejected RESET ALL changes neither values nor undo.
  • Explicit exclusions: file-backed descriptor/template reload, opaque proto graphs, connection identity, stream/output handles, unimplemented placeholders.

Validation

  • Focused unit tests for capture timing, aliases/case, exclusions, txn/batch guards, equal-value undo, rejected partial undo, and active output streams.
  • make check passed locally on this head (Docker + golangci-lint v2.13.2).
Open in Web Open in Cursor 

cursoragent and others added 2 commits September 14, 2026 12:30
Capture reset baselines after defaults, config, flags and --set, before
both init-command forms. Prepare and validate changed supported values
without mutating the live registry, then commit assignments and retire
targeted LOCAL undo only after whole-operation success.

Fixes #484

Co-authored-by: apstndb <apstndb@users.noreply.github.com>
Co-authored-by: apstndb <apstndb@users.noreply.github.com>
@apstndb
apstndb marked this pull request as ready for review September 14, 2026 12:34
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

Code Metrics Report

📊 View detailed coverage report (available for 7 days)

main (31f52f5) #976 (ed860c5) +/-
Coverage 86.8% 86.5% -0.4%
Code to Test Ratio 1:2.2 1:2.2 +0.0
Test Execution Time 2m0s 2m1s +1s
Details
  |                     | main (31f52f5) | #976 (ed860c5) |  +/-  |
  |---------------------|----------------|----------------|-------|
- | Coverage            |          86.8% |          86.5% | -0.4% |
  |   Files             |            112 |            113 |    +1 |
  |   Lines             |          10486 |          10674 |  +188 |
+ |   Covered           |           9112 |           9237 |  +125 |
+ | Code to Test Ratio  |          1:2.2 |          1:2.2 |  +0.0 |
  |   Code              |          23491 |          23845 |  +354 |
+ |   Test              |          53348 |          54175 |  +827 |
- | Test Execution Time |           2m0s |           2m1s |   +1s |

Code coverage of files in pull request scope (88.7% → 87.0%, patch 60.6%)

Files Coverage +/- Patch Coverage Status
internal/mycli/client_side_statement_def.go 91.0% +0.0% 100.0% modified
internal/mycli/config.go 90.9% +0.7% 33.3% modified
internal/mycli/feature/llm/llm.go 73.0% -3.6% 0.0% modified
internal/mycli/session.go 85.1% 0.0% - modified
internal/mycli/statements_system_variable.go 92.6% -0.5% 75.0% modified
internal/mycli/system_variables.go 96.5% 0.0% - modified
internal/mycli/transaction_manager.go 87.9% +0.0% 87.5% modified
internal/mycli/var_custom_handlers.go 79.2% -3.8% 35.4% modified
internal/mycli/var_defs.go 85.7% -10.3% 23.9% modified
internal/mycli/var_enum_handlers.go 82.0% -6.2% 52.6% modified
internal/mycli/var_errors.go 33.3% 0.0% - modified
internal/mycli/var_handler.go 87.1% -6.8% 50.0% modified
internal/mycli/var_reset.go 86.2% +86.2% 83.0% added

Reported by octocov

Co-authored-by: apstndb <apstndb@users.noreply.github.com>
@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Reviewed fbda8c233e5e8cc999da9b7b83d54820f33dc79d.

[P2] Compare the writable TRANSACTION_TAG slot, not the SHOW value, when deciding whether RESET is unchanged. The equality check in internal/mycli/var_reset.go uses rv.v.Get(). For TRANSACTION_TAG, Get intentionally reports the applied physical RW tag while the next-owner slot has already been consumed. These are different states.

Independently reproduced with the existing withFakeReadWriteOwner lifecycle helper (no backend claims):

  • Nonempty startup tag startup-tag, followed by a RW owner using it: SHOW remains startup-tag but the next-owner slot is empty. RESET ALL incorrectly succeeds and skips restoring that consumed startup slot. Since restoration would write the slot during an active RW owner, the existing transaction-tag guard should reject the complete reset without changing values/undo.
  • Normal empty startup tag, followed by a RW owner tagged applied-tag: the next-owner slot already equals the empty startup value. RESET ALL instead compares SHOW=applied-tag, invokes the guard, and wrongly rejects resetting an unrelated changed CLI_VERBOSE.

Use explicit reset snapshot/equality semantics for this handler, preserving the locked transaction-tag guard and the general skip-unchanged-before-guards rule. Do not change SHOW semantics to fix RESET. Cover both directions, pending/RO owners, and LOCAL undo behavior; after a failed complete reset, no values or undo entries may change. This is exactly the custom-state case that needs tested reset support rather than assuming every display string represents the mutable setting.

The existing focused RESET/capture tests passed independently. Both additional Go-overlay probes fail as described; tracked files remain unchanged. Please also integrate current main additively before final validation, run full make check (capture output and actual exit) before pushing, and own all applicable CI.

RESET capture and unchanged-equality now use ResetSnapshot for
TRANSACTION_TAG so SHOW (applied RW tag) is not mistaken for the
next-owner slot. Consumed nonempty startup tags hit the existing
RW guard; empty startup that already matches the consumed slot
skips the guard and still resets unrelated variables.

Co-authored-by: apstndb <apstndb@users.noreply.github.com>
@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Fixed in e359a2aa463ff953b9f3afa334dfca6681c4d2c8 (P2: compare the writable TRANSACTION_TAG next-owner slot, not SHOW).

RESET capture and unchanged-equality now use ResetSnapshot() on the TRANSACTION_TAG handler. SHOW still reports the applied RW tag. A consumed nonempty startup slot hits the existing RW guard and leaves values/undo unchanged; an already-empty slot matching empty startup skips that guard so unrelated RESET ALL assignments can proceed.

@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Independent coordinator confirmation on exact HEAD e359a2aa463ff953b9f3afa334dfca6681c4d2c8 (base 92f986d6a7ab74566bee82f318b8c44e9f340820).

Codex P2 is in that HEAD: RESET compares the writable TRANSACTION_TAG next-owner slot via ResetSnapshot(), not SHOW. All applicable hosted checks succeeded on that SHA (test, lint, race, readme-sync, govulncheck, all four cross-compile jobs). Dependabot automerge skipped. MERGEABLE CLEAN. No unresolved review threads. Worker local make check and make check-race exit 0.

{"repo":"apstndb/spanner-mycli","pr":976,"head":"e359a2aa463ff953b9f3afa334dfca6681c4d2c8","base":"92f986d6a7ab74566bee82f318b8c44e9f340820","validation":["hosted test/lint/race/readme-sync/govulncheck/cross-compile success on exact HEAD","local make check exit 0","local make check-race exit 0","MERGEABLE CLEAN","no unresolved review threads"],"deviations":[],"blockers":[]}

@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Independent review of HEAD e359a2aa463ff953b9f3afa334dfca6681c4d2c8: no actionable findings remain. RESET now captures/compares the mutable next-owner transaction-tag slot while SHOW continues to expose the applied RW tag. The reported false-equality and false-inequality cases are fixed, with pending/RO and LOCAL undo regressions covered.

Both independent transaction-tag overlay probes, focused RESET/capture tests, full Docker make check, and make check-race passed on this exact HEAD.

Main advanced through #974 during validation. Please perform the additive main merge for final integration, preserving both DDL_EXECUTION_MODE/DDL_ASYNC_WAIT_TIMEOUT reset support and the tag fix, run the mandatory gate before push, and handle all CI. Codex will verify that integration HEAD before merging.

@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Withdrawing the ready JSON for e359a2aa463ff953b9f3afa334dfca6681c4d2c8. Main moved to 31f52f5330ec67336d832a9b6da1e1c2b562617c (#974). This PR is MERGEABLE but BEHIND; the branch is being additively merged with origin/main. Do not squash-merge until a new HEAD is independently CI-green.

Co-authored-by: apstndb <apstndb@users.noreply.github.com>
@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Additive-merged origin/main #974 (31f52f5330ec67336d832a9b6da1e1c2b562617c). Merge HEAD is 3c1d006dec0e855afc96a98a924e7dafc5ba7b7a.

README, docs, and var_defs.go auto-merged additively. TRANSACTION_TAG RESET still compares ResetSnapshot() (writable next-owner slot), not SHOW. Local make check exit 0.

@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Confirmed on merge HEAD 3c1d006dec0e855afc96a98a924e7dafc5ba7b7a (base 31f52f5330ec67336d832a9b6da1e1c2b562617c):

  • TRANSACTION_TAG RESET still uses ResetSnapshot() (writable next-owner slot), not SHOW.
  • DDL_EXECUTION_MODE and DDL_ASYNC_WAIT_TIMEOUT remain resettable and restore from startup snapshots (--set ASYNC/15s survived RESET ALL after a later SET).

Shared README/docs/var_defs.go stayed additive. No rebase/force-push. Local make check exit 0 (make-check-7.log).

@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Independent coordinator confirmation on exact HEAD 3c1d006dec0e855afc96a98a924e7dafc5ba7b7a (base 31f52f5330ec67336d832a9b6da1e1c2b562617c, #974).

Additive main merge. Worker reports TRANSACTION_TAG RESET still uses ResetSnapshot(), and DDL_EXECUTION_MODE / DDL_ASYNC_WAIT_TIMEOUT remain resettable. All applicable hosted checks succeeded on that SHA (test, lint, race, readme-sync, govulncheck, all four cross-compile jobs). Dependabot automerge skipped. MERGEABLE CLEAN. No unresolved review threads. Prior ready JSON on e359a2a stays withdrawn.

{"repo":"apstndb/spanner-mycli","pr":976,"head":"3c1d006dec0e855afc96a98a924e7dafc5ba7b7a","base":"31f52f5330ec67336d832a9b6da1e1c2b562617c","validation":["hosted test/lint/race/readme-sync/govulncheck/cross-compile success on exact HEAD","worker local make check exit 0","MERGEABLE CLEAN","no unresolved review threads"],"deviations":[],"blockers":[]}

@apstndb

apstndb commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Independent review of integration HEAD 3c1d006dec0e855afc96a98a924e7dafc5ba7b7a: no actionable findings remain. Its tree exactly matches the automatic merge of accepted RESET HEAD e359a2aa463ff953b9f3afa334dfca6681c4d2c8 and reviewed main 31f52f5330ec67336d832a9b6da1e1c2b562617c; there are no manual conflict edits.

An additional integration probe passed on this HEAD: startup --set overrides for DDL_EXECUTION_MODE and DDL_ASYNC_WAIT_TIMEOUT are restored by RESET ALL after both are changed. The original transaction-tag reproductions, focused reset tests, full Docker make check and make check-race passed independently on the accepted feature HEAD; those results remain attributed there. All nine applicable current-head CI checks passed, the conditional Dependabot automerge job is skipped, and no unresolved review feedback remains.

@apstndb
apstndb merged commit 83ae3ab into main Sep 14, 2026
10 checks passed
@apstndb
apstndb deleted the cursor/reset-all-startup-484 branch September 14, 2026 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement RESET ALL with startup snapshots and atomic validation

2 participants