Fix two engine deadlocks, and make the next one loud

.github/workflows/ci.yaml

@@ -36,11 +36,19 @@ jobs:
           go-version-file: "go.mod"
       - name: go tests
         if: ${{ matrix.platform != 'windows-latest' }}
-        run: go test -tags=baton_lambda_support -v -covermode=count -json ./... > test.json
+        run: go test -tags=baton_lambda_support,baton_lockchecks -v -covermode=count -json ./... > test.json
       - name: go tests
         if: ${{ matrix.platform == 'windows-latest' }}
         # Run tests with -short on Windows since its filesystem is very slow, causing CI to time out.
-        run: go test -timeout=30m -tags=baton_lambda_support -short -v -covermode=count -json ./... > test.json
+        #
+        # bash, not the Windows default of pwsh: PowerShell treats an
+        # unquoted comma as the array operator, so the comma-separated tag
+        # list made this step fail in zero seconds without even creating
+        # test.json, which then surfaced as a confusing "cat test.json"
+        # failure two steps later. bash also keeps the two legs' quoting
+        # rules identical, so a flag that works on one works on the other.
+        shell: bash
+        run: go test -timeout=30m -tags=baton_lambda_support,baton_lockchecks -short -v -covermode=count -json ./... > test.json
       - name: Print go test results
         if: always()
         run: cat test.json

.github/workflows/main.yaml

@@ -51,11 +51,19 @@ jobs:
           go-version-file: "go.mod"
       - name: go tests
         if: ${{ matrix.platform != 'windows-latest' }}
-        run: go test -tags=baton_lambda_support -v -covermode=count -json ./... > test.json
+        run: go test -tags=baton_lambda_support,baton_lockchecks -v -covermode=count -json ./... > test.json
       - name: go tests
         if: ${{ matrix.platform == 'windows-latest' }}
         # Run tests with -short on Windows since its filesystem is very slow, causing CI to time out.
-        run: go test -timeout=30m -tags=baton_lambda_support -short -v -covermode=count -json ./... > test.json
+        #
+        # bash, not the Windows default of pwsh: PowerShell treats an
+        # unquoted comma as the array operator, so the comma-separated tag
+        # list made this step fail in zero seconds without even creating
+        # test.json, which then surfaced as a confusing "cat test.json"
+        # failure two steps later. bash also keeps the two legs' quoting
+        # rules identical, so a flag that works on one works on the other.
+        shell: bash
+        run: go test -timeout=30m -tags=baton_lambda_support,baton_lockchecks -short -v -covermode=count -json ./... > test.json
       - name: Print go test results
         if: always()
         run: cat test.json

.golangci.yml

@@ -2,6 +2,13 @@ version: "2"
 run:
   build-tags:
     - baton_lambda_support
+    # Files behind this tag are excluded from every linter without it, so
+    # the deadlock-check instrumentation and the tests that assert it
+    # would be the least-reviewed code in the tree.
+    - baton_lockchecks
+    # Files behind this tag are excluded from every linter without it, so
+    # the deadlock-check instrumentation and the tests that assert it
+    # would be the least-reviewed code in the tree.
 linters:
   default: none
   enable:

.vscode/settings.json

@@ -1,3 +1,3 @@
 {
-    "go.buildTags": "baton_lambda_support"
+    "go.buildTags": "baton_lambda_support,baton_lockchecks"
 }

CLAUDE.md

@@ -18,11 +18,13 @@ make frontend
 # Run linter (golangci-lint v2)
 make lint
 
-# Run all tests
-go test -v ./...
+# Run all tests. The baton_lockchecks tag compiles in the pebble engine's
+# deadlock-shape checks; a tripwire test fails any whole-tree run without it.
+# -race arms the same checks without the tag.
+go test -tags=baton_lockchecks -v ./...
 
 # Run a single test
-go test -v -run TestName ./path/to/package
+go test -tags=baton_lockchecks -v -run TestName ./path/to/package
 
 # Update dependencies (updates, tidies, and vendors)
 make update-deps

Makefile

@@ -52,7 +52,7 @@ protofmt: ## Format protobuf definitions.
 
 .PHONY: test
 test: ## Run the Go test suite used by CI.
-	go test -tags=baton_lambda_support -v ./...
+	go test -tags=baton_lambda_support,baton_lockchecks -v ./...
 
 # Two-artifact checkpoint compatibility matrix: builds the harness against
 # HEAD and a pinned past release, and exchanges mid-flight checkpoints in
@@ -82,14 +82,14 @@ compat-check: ## Exchange checkpoints with a pinned older SDK.
 # uninterrupted baseline. See cmd/baton-crash-harness.
 .PHONY: crash-check
 crash-check: ## Exercise cross-process checkpoint/resume under hard kills.
-	BATON_DEMO_CRASH=1 go test -v -count=1 -timeout=30m -run TestCrashResumeRealConnector ./cmd/baton-crash-harness
+	BATON_DEMO_CRASH=1 go test -tags=baton_lockchecks -v -count=1 -timeout=30m -run TestCrashResumeRealConnector ./cmd/baton-crash-harness
 
 .PHONY: demo-crash-check
 demo-crash-check: crash-check ## Deprecated alias for crash-check.
 
 .PHONY: checkpoint-cut-check
 checkpoint-cut-check: ## Resume from every durable checkpoint cut.
-	BATON_CUT_SWEEP=full go test -v -count=1 -timeout=30m -run TestCheckpointCutEnumeration ./pkg/sync
+	BATON_CUT_SWEEP=full go test -tags=baton_lockchecks -v -count=1 -timeout=30m -run TestCheckpointCutEnumeration ./pkg/sync
 
 .PHONY: interrupt-check
 interrupt-check: checkpoint-cut-check crash-check ## Run in-process cut and real-process interruption checks.
@@ -223,7 +223,7 @@ fuzz-smoke: ## Run each native Go fuzzer for FUZZ_TIME (default 30s).
 
 .PHONY: differential-check
 differential-check: ## Differential-fuzz SQLite and Pebble for DIFFERENTIAL_TIME.
-	BATON_EXPAND_FUZZ_DURATION=$(DIFFERENTIAL_TIME) go test -v -count=1 -timeout=30m -run '^TestFullPipelineDifferentialFuzz$$' ./pkg/sync/expand
+	BATON_EXPAND_FUZZ_DURATION=$(DIFFERENTIAL_TIME) go test -tags=baton_lockchecks -v -count=1 -timeout=30m -run '^TestFullPipelineDifferentialFuzz$$' ./pkg/sync/expand
 
 .PHONY: bench-smoke
 bench-smoke: ## Run the bounded checkpoint cost benchmarks once.
@@ -240,7 +240,7 @@ scheduler-soak: ## Run randomized scheduler cases under race detection.
 
 .PHONY: errorfs-soak
 errorfs-soak: ## Sweep whole-sync Pebble crash points using errorfs.
-	BATON_SOAK=1 go test -v -count=1 -timeout=30m -run TestErrorFSWholeSyncRandomSweepSoak ./pkg/dotc1z/engine/pebble
+	BATON_SOAK=1 go test -tags=baton_lockchecks -v -count=1 -timeout=30m -run TestErrorFSWholeSyncRandomSweepSoak ./pkg/dotc1z/engine/pebble
 
 .PHONY: chaos-check
 chaos-check: ## Run bounded representative chaos checks under race detection.
@@ -270,9 +270,17 @@ test-nightly: ## Run extended confidence, fuzz, scheduler, and errorfs checks.
 # test-extra and test-nightly: they create multi-million-row fixtures and may
 # consume hours and substantial disk. Their BATON_* sizing variables remain
 # available as documented in docs/TESTING.md and the test files.
+#
+# Lock-check arming policy: every correctness-focused target above compiles
+# with -race (which arms the engine's deadlock-shape checks by itself) or
+# with -tags=baton_lockchecks. The pure measurement targets — bench*,
+# prodscale-crossover, prodscale-topebble — stay unarmed so the numbers they
+# exist to produce are not skewed by instrumentation, and compat-check stays
+# unarmed because it also builds a pinned past release that may predate the
+# tag. prodscale-check is a correctness experiment first, so it is armed.
 .PHONY: prodscale-check
 prodscale-check: ## Run the multi-million-row compactor experiment.
-	BATON_PROD_SCALE_TEST=1 go test -v -count=1 -timeout=60m -run 'TestProdScale' ./pkg/synccompactor
+	BATON_PROD_SCALE_TEST=1 go test -tags=baton_lockchecks -v -count=1 -timeout=60m -run 'TestProdScale' ./pkg/synccompactor
 
 .PHONY: prodscale-crossover
 prodscale-crossover: ## Measure fold/overlay crossover at production scale.

docs/TESTING.md

@@ -8,10 +8,18 @@ Run `make help` for the current target list.
 
 ## CI-equivalent tests
 
-`make test` runs the ordinary Go test suite with the same build tag used by
+`make test` runs the ordinary Go test suite with the same build tags used by
 CI. Pull-request CI also runs lint, protobuf checks, the full build, and
 `make race-shard-audit` (see [Race shards](#race-shards)).
 
+One of those tags is `baton_lockchecks`, which compiles in the Pebble
+engine's deadlock-shape checks (`pkg/dotc1z/engine/pebble/lock_checks_enabled.go`)
+and the tests that assert them. `-race` arms the same checks without the
+tag, so the race-based targets need no opt-in. A bare `go test ./...`
+fails on `TestLockChecksCompiledIn` by design — that failure is the only
+sign the checks and their tests were silently excluded. Benchmarks are
+the one intended unarmed run; use `-bench` with `-run='^$'`.
+
 Tests in this tier should be deterministic, self-contained, and reasonably
 fast. A test that only skips on Windows with `testing.Short()` is still a CI
 test on the other platforms.

docs/verification/engine-close-gate/evidence.md

@@ -0,0 +1,163 @@
+# Evidence record: pebble engine close gate — review-closure stage
+
+Findings source: three independent model reviews of PR #1088, consolidated
+and re-verified against the code before any fix. Each entry names the
+finding, the fix, and the instrument that now holds it.
+
+## Findings fixed in this stage
+
+1. Unpinned point-read surface (all three reviewers). `GetGrantRecord`,
+   `GetEntitlementRecord`, `GetResourceRecord`, `GetResourceTypeRecord`,
+   `GetAssetRecord`, `GetSyncRunRecord`, `readSyncStats`, `SessionGet`,
+   `SessionGetMany`, `sessionGetAllChunk`, `computeSyncStats`, the digest
+   read surface (`GetEntitlementDigestRoot`, `GetGrantDigestGlobalRoot`,
+   `ComputeEntitlementBucketDigest`, `GetEntitlementGrantDigestNodes`,
+   `dirtyPartitionBuckets` — both engines), `EnsureGrantIndexes`'s pending
+   probe, and the `GrantDigestsPresent` checks in the digest repair path
+   read `e.db` with no admission, so a concurrent Close could tear the
+   handle down mid-read. Fix: pin at every entry point; plumb the admitted
+   handle through the resolve/digest/repair helper chains
+   (`lookup.go`, `digest.go`, `ingest_facts.go`, `ingest_repair.go`) so one
+   admission covers the whole operation and probes inside pinned scans
+   cannot be refused mid-scan by a re-pin racing the flip.
+   Instrument: `TestBareHandleAccessIsGateCovered`.
+
+2. Nested-pin refusal inside admitted scans (one reviewer).
+   `ForEachDanglingGrantPrincipal` → `HasResourceRecord` re-pinned inside a
+   pinned scan. Fix: `hasResourceRecordOn`/`hasEntitlementIdentity`/
+   `grantIdentitiesForPrincipal`/`getGrantRecordByIdentity` take the
+   caller's handle. Instrument: same as (1); behavior covered by the
+   existing dangling-referent tests.
+
+3. Racy, redundant `e.db == nil` checks (one reviewer).
+   `checkWritableAllowSealed` and `EnsureGrantIndexes` read the field
+   outside the gate as a pseudo-lifecycle check; `pinRead` kept a dead nil
+   branch. Fix: deleted (with the reasoning written in place); the closing
+   flag and gate ordering are the guarantee. The merge surface's nil checks
+   stay, re-documented as sequential post-close misuse guards under the
+   compactor ordering fence.
+
+4. Lifecycle transitions outside the gate + ResumeSync TOCTOU (two
+   reviewers). Transitions did bare handle reads Close could race, and
+   ResumeSync validated the sync-run record before taking `lifecycleMu`.
+   Fix: all five transitions assert-then-lock and run as admitted writes;
+   ResumeSync validates under the lock.
+   Instrument: `TestLifecycleMuTakersAreTransitionsOnly` (ordering enforced
+   by token position).
+
+5. WaitGroup Add-vs-Wait misuse in the gate (two reviewers), plus the same
+   latent bug in `CompactAllRanges`/`Flush`. Fix: the gate counts under
+   `countMu` and signals a condition variable; drains tolerate concurrent
+   enters by construction. Instruments:
+   `TestAdmissionDrainWritesToleratesConcurrentEnters`,
+   `TestAdmissionEnterNeverTripsDrainingWaitGroup`.
+
+6. Enforcement holes in the meta-tests (all three reviewers, different
+   pieces). Name-prefix keying missed non-family reads; `pinRead` release
+   discipline was unchecked; seek-driven iterator loops
+   (`for valid := iter.First(); valid;`) were invisible to the ctx-check
+   rule. Fix: `TestBareHandleAccessIsGateCovered` (keys on the field
+   access), `TestPinnedReadsDeferTheirRelease`, and the extended
+   `scanLoopCancellation`.
+
+7. Unarmed Make targets (one reviewer). errorfs-soak, crash-check,
+   checkpoint-cut-check, differential-check, prodscale-check compiled the
+   engine without the deadlock-shape checks. Fix: armed; the per-target
+   policy (and why bench/crossover/topebble/compat stay unarmed) is a
+   comment in the Makefile.
+
+8. Stale documentation (two reviewers): the pre-pin reader paragraph on
+   `TestConcurrentCloseWithPaginatedReads`, and Close's unqualified
+   panic-instead-of-hang claim (true only in armed builds). Rewritten.
+
+## Findings fixed in the follow-up stage (unresolved PR review threads)
+
+Re-verified against the code before fixing; two threads on the same file
+were left to main, which had already landed a better version of them (see
+"Deferred to main" below).
+
+9. Windows-only failure in the arming tripwire (blocking).
+   `TestLockChecksSuppliedByTestInvocations` classified config files by
+   `strings.HasPrefix(rel, ".github/")`, but `filepath.Rel` yields
+   backslashes on Windows, so the workflow floor counted zero hits and the
+   test failed the run — on a tree with nothing wrong with it. CI runs
+   `./...` on `windows-latest` with the tag and no `-short` skip on this
+   test, so it was reachable. Fix: `filepath.ToSlash` the relative path,
+   and fail loudly on a `filepath.Rel` error instead of keying the map on
+   an empty string.
+
+10. Unbounded retry in `CurrentSyncStep` (one reviewer). The
+    generation-recheck loop retries until a pass sees a stable binding;
+    nothing in it consults the caller's context, so the termination
+    argument — transitions run out — was the only thing keeping it from
+    spinning forever. Fix: check `ctx.Err()` on every pass after the
+    first. The first pass stays unguarded so a caller reading the step
+    while shutting down (the expiry checkpoint does) still gets an answer.
+    Instrument: `TestCurrentSyncStepRetryHonorsCancellation`.
+
+11. Tag-gated files were unlinted (one reviewer). `.golangci.yml` listed
+    only `baton_lambda_support`, so every linter skipped the lock-check
+    instrumentation and its tests — the least-reviewed code in the tree
+    was the code asserting the concurrency contract. Fix: added
+    `baton_lockchecks` to `run.build-tags`. This surfaced the unused
+    `lineNo` in the tripwire, now folded into the violation message as
+    `file:line: invocation`, which is what a reader needs anyway.
+
+12. After-Close coverage stopped at the scan families (one reviewer).
+    `TestReadSurfaceAfterCloseReturnsClosing` covered Paginate and
+    Iterate; the point reads pinned in this PR had no lifecycle
+    assertion, and they are the quieter failure — no iterator, so nothing
+    on the path that happens to check. Fix: extended the table with 13
+    point reads (the `Get*` family, `HasResourceRecord`,
+    `ComputeEntitlementBucketDigest`, `GetEntitlementGrantDigestNodes`,
+    `SessionGet`, `SessionGetMany`).
+
+## Deferred to main
+
+The two remaining threads were both on `pkg/sync/type_scoped_test.go`,
+whose run-duration work landed on main separately as #1091. Main's
+`flattenJoined` already peels single-error wrappers while looking for the
+join — the exact degradation the thread described — and carries
+`TestFlattenJoinedSeesWrappedJoins` to hold it. This branch's copy was the
+older version, so the rebase resolves the file to main's side and the
+branch no longer touches `pkg/sync` at all.
+
+## Instrument liveness (mutation evidence)
+
+Each new instrument was shown to fail against a seeded defect before
+closure was claimed:
+
+- `TestBareHandleAccessIsGateCovered`: run before the fixes were allowlisted,
+  it reported the then-real violations (`computeSyncStats`, the digest
+  repair checks, `endSyncFinalize`, and the build/repair helper family)
+  — the allowlist was populated only after each entry's admission was
+  verified by reading its callers.
+- `TestPinnedReadsDeferTheirRelease`: mutating `GetAssetRecord`'s
+  `defer release()` to a bare `release()` failed the test with the
+  expected message; reverted.
+- `scanLoopCancellation` extension: deleting the `ctx.Err()` check from
+  `ForEachDanglingGrantPrincipal`'s seek-driven loop failed
+  `TestScanReadsArePinned/ForEachDanglingGrantPrincipal`; reverted.
+- `TestCurrentSyncStepRetryHonorsCancellation`: removing the `pass > 0`
+  cancellation check made the test report the spin at its own 30s budget
+  rather than hanging the binary until the package timeout; reverted.
+- Point-read after-Close coverage: unpinning `GetResourceTypeRecord` (bare
+  `e.db`) was caught twice over — `TestBareHandleAccessIsGateCovered`
+  named `resource_types.go:57 GetResourceTypeRecord`, and the new
+  `TestReadSurfaceAfterCloseReturnsClosing/GetResourceTypeRecord` caught
+  the nil dereference the pin prevents; reverted.
+- Windows path handling: replacing the `".github/"` prefix with
+  `".github\\"` — what the un-normalized path would have matched on
+  Windows — reproduced the reported failure verbatim ("no whole-tree
+  `go test ./...` line found in any CI workflow"), confirming the floor
+  assertion is what fires and that `ToSlash` is what prevents it;
+  reverted.
+
+## Suite evidence
+
+On the final tree: `go build -tags=baton_lambda_support,baton_lockchecks
+./...` clean; `golangci-lint run ./pkg/dotc1z/engine/pebble/...` zero
+issues; `go test -race -tags=baton_lockchecks -count=1
+./pkg/dotc1z/engine/pebble/ ./pkg/dotc1z/ ./pkg/synccompactor/...` pass
+(the engine package alone is ~101s under -race; results recorded in the PR
+checks on push).