diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 69ae1fbe8..b1ba2eabe 100644 --- a/.github/workflows/ci.yaml +++ b/.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 diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index d9cea6959..f7bcd7a3b 100644 --- a/.github/workflows/main.yaml +++ b/.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 diff --git a/.golangci.yml b/.golangci.yml index 20cb0990a..0ff3b70ed 100644 --- a/.golangci.yml +++ b/.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: diff --git a/.vscode/settings.json b/.vscode/settings.json index 3d2addb54..afb6376f4 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,3 +1,3 @@ { - "go.buildTags": "baton_lambda_support" + "go.buildTags": "baton_lambda_support,baton_lockchecks" } \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index 909f4b575..24959efc8 100644 --- a/CLAUDE.md +++ b/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 diff --git a/Makefile b/Makefile index 863809573..1e22848f6 100644 --- a/Makefile +++ b/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. diff --git a/docs/TESTING.md b/docs/TESTING.md index 1acc505f8..2a92ef0cb 100644 --- a/docs/TESTING.md +++ b/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. diff --git a/docs/verification/engine-close-gate/evidence.md b/docs/verification/engine-close-gate/evidence.md new file mode 100644 index 000000000..c606e1b36 --- /dev/null +++ b/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). diff --git a/docs/verification/engine-close-gate/plan.md b/docs/verification/engine-close-gate/plan.md new file mode 100644 index 000000000..8c91232fd --- /dev/null +++ b/docs/verification/engine-close-gate/plan.md @@ -0,0 +1,84 @@ +# Verification plan: pebble engine close gate (admission) — review-closure stage + +Stage of the `kans/engine-deadlock-fixes` series (PR #1088). Earlier commits +on the branch introduced the read pin, the write-side close drain, the +owned-mutex diagnostics, and consolidated seven concurrency fields into one +`admission` type. This stage closes the findings of a three-reviewer code +pass over that consolidated state. Proportionality per BUG_CATCHING §"keep +the machinery proportional": this packet covers the delta, not a re-plan of +the whole subsystem; the enduring enforcement lives in the repository tests +named below, which survive this document. + +## Contracts under verification + +- C1 (gate soundness): no code path reads the engine's `db` handle without + holding gate admission — a read pin, an admitted write, lifecycle + machinery that is itself the admission, Open-time code before the engine + is shared, or the merge surface's documented exclusion. +- C2 (drain correctness): `Close` waits for every admitted operation and + runs the teardown exactly once; an admission attempt concurrent with the + closing flip either completes against a live handle or is refused with + `ErrEngineClosing`. Draining must tolerate concurrent enter attempts + (the `sync.WaitGroup` Add-vs-Wait misuse is structurally excluded: + the gate counts under a mutex and signals on a condition variable). +- C3 (lifecycle transitions): all five transitions (StartNewSync, + ResumeSync, SetCurrentSync, CheckpointSync, EndSync) run as admitted + writes, assert the write barrier is not held before taking + `lifecycleMu`, and validate state under the lock (no check-then-bind + TOCTOU against a concurrent wipe). +- C4 (no leaked admissions): every pin's release is deferred; an early + return cannot strand a counter and wedge Close. +- C5 (bounded pins): every pinned iterator scan checks `ctx.Err()` per + iteration, including the seek-driven distinct-referent shape. +- C6 (armed builds): correctness-focused Make targets compile the + deadlock-shape checks in, either via `-race` or `baton_lockchecks`; + measurement targets stay unarmed, stated per target. The tag is also in + `.golangci.yml`'s build tags, or the instrumentation and the tests + asserting it are the only unlinted code in the package. +- C7 (post-close surface): after `Close`, every exported read returns + `ErrEngineClosing` rather than answering from a torn-down handle — + point reads included, not just the scan families that crashed loudly. +- C8 (bounded retries): the lock-free `CurrentSyncStep` re-read loop + terminates on caller cancellation, so its liveness does not rest on the + assumption that lifecycle transitions stop arriving. + +## Coverage model + +Mechanical, not sampled: + +- C1: `TestBareHandleAccessIsGateCovered` (AST) enumerates every `e.db` / + `*.e.db` selector in the package and requires a withWrite literal or a + justified allowlist entry. `TestAdmissionUsedOnlyThroughItsMethods` pins + the gate's internals to `admission.go`. +- C2: `admission_test.go` unit-hammers the gate directly + (`TestAdmissionDrainWritesToleratesConcurrentEnters`, + `TestAdmissionEnterNeverTripsDrainingWaitGroup`); + `TestCloseWaitsForInFlightAdmission` white-boxes the flip/enter race; + `TestConcurrentCloseWithPaginatedReads` and the write-side reentry tests + hammer the engine end to end under `-race`. +- C3: `TestLifecycleMuTakersAreTransitionsOnly` (AST) enforces the + transition set and the assert-before-lock ordering by token position. +- C4: `TestPinnedReadsDeferTheirRelease` (AST) covers every `pinRead` + call site. +- C5: `TestScanReadsArePinned` (AST) covers both iterator-loop shapes + (`iter.Valid()` conditions and seek-driven bool conditions). +- C6: reviewed target-by-target in the Makefile; the arming policy is + written beside the targets. `TestLockChecksCompiledIn` fails an unarmed + run and `TestLockChecksSuppliedByTestInvocations` fails the diff that + de-arms a whole-tree invocation, with floor assertions so a restructure + cannot leave it matching nothing. +- C7: `TestReadSurfaceAfterCloseReturnsClosing` enumerates the surface as + a subtest table, so one run names every method that regressed; + `TestIngestScanSurfaceAfterCloseReturnsClosing` covers the + invariant-scan family. +- C8: `TestCurrentSyncStepRetryHonorsCancellation` drives the retry branch + through the pre-read seam indefinitely and requires the call to return. + +## Closure + +Closure for this stage is: the meta/unit instruments above pass, the +package suite passes under `-race -tags=baton_lockchecks`, and each +instrument has been shown live by mutation (see evidence.md). Deferred +beyond this stage: object-tied leases for the merge surface (documented +exclusion instead — see merge_surface.go header), and any redesign of the +bare-ID lookup's O(all grants) fallback (pre-existing, documented). diff --git a/pkg/dotc1z/c1file_concurrent_test.go b/pkg/dotc1z/c1file_concurrent_test.go deleted file mode 100644 index 32fb8fa90..000000000 --- a/pkg/dotc1z/c1file_concurrent_test.go +++ /dev/null @@ -1,163 +0,0 @@ -package dotc1z - -import ( - "fmt" - "os" - "path/filepath" - "sync" - "testing" - "time" - - "github.com/stretchr/testify/require" - - v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" - "github.com/conductorone/baton-sdk/pkg/connectorstore" -) - -func TestC1ZConcurrentClose(t *testing.T) { - ctx := t.Context() - - testFilePath := filepath.Join(c1zTests.workingDir, "test-concurrent-close.c1z") - - f, err := NewC1ZFile(ctx, testFilePath, WithPragma("journal_mode", "WAL")) - require.NoError(t, err) - - syncID, err := f.StartNewSync(ctx, connectorstore.SyncTypeFull, "") - require.NoError(t, err) - require.NotEmpty(t, syncID) - - err = f.PutResourceTypes(ctx, v2.ResourceType_builder{Id: testResourceType}.Build()) - require.NoError(t, err) - - // Add a bunch of resources, entitlements, and grants to fill up the WAL file. - userCount := 100 - resourceCount := 1500 - entitlementsPerResource := 10 - grantsPerEntitlement := 10 - - users := []*v2.Resource{} - for i := range userCount { - user := v2.Resource_builder{ - Id: v2.ResourceId_builder{ - ResourceType: testResourceType, - Resource: fmt.Sprintf("user-%07d", i), - }.Build(), - }.Build() - users = append(users, user) - } - err = f.PutResources(ctx, users...) - require.NoError(t, err) - - resources := []*v2.Resource{} - for i := range resourceCount { - resource := v2.Resource_builder{ - Id: v2.ResourceId_builder{ - ResourceType: testResourceType, - Resource: fmt.Sprintf("resource-%07d", i), - }.Build(), - }.Build() - resources = append(resources, resource) - entitlements := []*v2.Entitlement{} - for j := range entitlementsPerResource { - entitlement := v2.Entitlement_builder{ - Id: fmt.Sprintf("entitlement-r%07d-%07d", i, j), - Resource: resource, - }.Build() - entitlements = append(entitlements, entitlement) - grants := []*v2.Grant{} - for k := range grantsPerEntitlement { - grants = append(grants, v2.Grant_builder{ - Id: fmt.Sprintf("grant-r%07d-%07d-%07d", i, j, k), - Principal: users[k%userCount], - Entitlement: entitlement, - }.Build()) - } - err = f.PutGrants(ctx, grants...) - require.NoError(t, err) - } - err = f.PutEntitlements(ctx, entitlements...) - require.NoError(t, err) - } - - err = f.PutResources(ctx, resources...) - require.NoError(t, err) - - err = f.EndSync(ctx) - require.NoError(t, err) - - expectedGrantStats := map[string]int64{ - testResourceType: int64(resourceCount * entitlementsPerResource * grantsPerEntitlement), - } - - stats, err := f.grantStats(ctx, connectorstore.SyncTypeAny, syncID) - require.NoError(t, err) - for k, v := range expectedGrantStats { - require.Equal(t, v, stats[k]) - } - - start := time.Now() - // Close concurrently with a PutGrants operation. - wg := sync.WaitGroup{} - wg.Add(2) - closeFunc := func() { - defer wg.Done() - err := f.Close(ctx) - require.NoError(t, err) - elapsed := time.Since(start) - t.Logf("close took %s", elapsed) - } - - syncID, err = f.StartNewSync(ctx, connectorstore.SyncTypeFull, "") - require.NoError(t, err) - require.NotEmpty(t, syncID) - - putGrantsFunc := func() { - var err error - // Close will finish at some point, causing DB operations to fail. - defer wg.Done() - // Put grants in a loop until we get a DbNotOpen error. - i := 0 - for { - err = f.PutGrants(ctx, v2.Grant_builder{ - Id: fmt.Sprintf("grant-%d", i), - Principal: v2.Resource_builder{ - Id: v2.ResourceId_builder{ - ResourceType: testResourceType, - Resource: fmt.Sprintf("user-%07d", i%userCount), - }.Build(), - }.Build(), - Entitlement: v2.Entitlement_builder{ - Id: fmt.Sprintf("entitlement-r%07d-%07d", i%resourceCount, i%entitlementsPerResource), - Resource: v2.Resource_builder{ - Id: v2.ResourceId_builder{ - ResourceType: testResourceType, - Resource: fmt.Sprintf("resource-%07d", i%resourceCount), - }.Build(), - }.Build(), - }.Build(), - }.Build()) - if err != nil { - require.ErrorIs(t, err, ErrDbNotOpen) - break - } - i++ - } - t.Logf("grant insert count: %d", i) - err = f.EndSync(ctx) - require.ErrorIs(t, err, ErrDbNotOpen) - } - wg.Go(putGrantsFunc) - wg.Go(closeFunc) - - // Wait for both goroutines to finish. - wg.Wait() - - // Validate that the WAL file is nonexistent or empty. - walPath := f.dbFilePath + "-wal" - walInfo, err := os.Stat(walPath) - if err != nil { - require.ErrorIs(t, err, os.ErrNotExist, "WAL file should not exist") - } else { - require.Equal(t, int64(0), walInfo.Size(), "WAL file should be empty") - } -} diff --git a/pkg/dotc1z/engine/pebble/adapter.go b/pkg/dotc1z/engine/pebble/adapter.go index 272920cb2..71a2e6d39 100644 --- a/pkg/dotc1z/engine/pebble/adapter.go +++ b/pkg/dotc1z/engine/pebble/adapter.go @@ -96,6 +96,23 @@ func (e *Engine) startNewSync(ctx context.Context, syncType connectorstore.SyncT if syncID == "" { syncID = ksuid.New().String() } + // Guard BEFORE lifecycleMu, like every transition: the body's own + // writes would trip the barrier re-entrancy check eventually, but + // only after lifecycleMu is already held — and blocking on a + // contended lifecycleMu while holding the barrier IS the deadlock, + // so the check must fire before the lock, not at the first write. + e.assertNotTakingLifecycleFromWrite() + // The transition runs as one admitted write: Close either waits it + // out or refuses it, so the bare handle reads inside (hasSyncRun, + // the pre-wipe) can never race the teardown. Steps inside that + // re-enter the gate (ResetForNewSync, PutSyncRunRecord) nest — + // entries are counted, not owned. If Close flips the gate mid- + // transition, the next nested entry is refused and the transition + // unwinds with ErrEngineClosing through its normal error path. + if err := e.admit.enterWrite(); err != nil { + return "", err + } + defer e.admit.exitWrite() e.lifecycleMu.Lock() defer e.lifecycleMu.Unlock() // Single-sync contract: a v3 Pebble c1z holds exactly one sync. @@ -143,11 +160,22 @@ func (e *Engine) ResumeSync(ctx context.Context, syncType connectorstore.SyncTyp if syncID == "" { return "", errors.New("pebble.ResumeSync: empty syncID") } - if _, err := e.GetSyncRunRecord(ctx, syncID); err != nil { - return "", c1zstore.AdaptNotFound(fmt.Errorf("ResumeSync: lookup: %w", err), pebble.ErrNotFound) + e.assertNotTakingLifecycleFromWrite() + // Admitted as one write for the whole check-then-bind; see + // startNewSync for the contract. + if err := e.admit.enterWrite(); err != nil { + return "", err } + defer e.admit.exitWrite() e.lifecycleMu.Lock() defer e.lifecycleMu.Unlock() + // Validate UNDER lifecycleMu, not before it: startNewSync wipes the + // prior sync's record and binds the replacement under this lock, so + // a check made outside it can pass against a record the wipe is + // about to delete — and then bind a sync that no longer exists. + if _, err := e.GetSyncRunRecord(ctx, syncID); err != nil { + return "", c1zstore.AdaptNotFound(fmt.Errorf("ResumeSync: lookup: %w", err), pebble.ErrNotFound) + } if err := e.bindCurrentSync(syncID); err != nil { return "", err } @@ -196,6 +224,13 @@ func (e *Engine) StartOrResumeSync(ctx context.Context, syncType connectorstore. // whose step can't be read (SQLite's SetCurrentSync likewise returns // its getSync error). func (e *Engine) SetCurrentSync(ctx context.Context, syncID string) error { + e.assertNotTakingLifecycleFromWrite() + // Admitted as one write for the whole check-then-bind; see + // startNewSync for the contract. + if err := e.admit.enterWrite(); err != nil { + return err + } + defer e.admit.exitWrite() e.lifecycleMu.Lock() defer e.lifecycleMu.Unlock() if _, err := e.GetSyncRunRecord(ctx, syncID); err != nil && !errors.Is(err, pebble.ErrNotFound) { @@ -210,31 +245,85 @@ func (e *Engine) SetCurrentSync(ctx context.Context, syncID string) error { // step cache to go stale (the old Adapter cached it and grew // rehydration logic at every rebind to compensate). // -// Holds lifecycleMu so the id read and the record read are one -// snapshot with respect to the lifecycle transitions — without it, an -// interleaved EndSync/SetCurrentSync between the two reads could -// return a token for a sync the engine is no longer bound to (the old -// Adapter's mutex gave the same guarantee over its cache; review -// finding, final round). +// The id read and the record read have to agree on which sync is +// bound, or an interleaved EndSync/SetCurrentSync between them would +// return a token for a sync the engine has already left. This gets that +// agreement by re-reading the binding generation afterwards instead of +// holding lifecycleMu, and the difference is not a micro-optimization: +// EndSync holds lifecycleMu across a finalize whose steps take the +// write barrier, so taking it here made the order lifecycleMu → writeMu +// in one direction and writeMu → lifecycleMu in the other for any write +// whose body read the current step. That deadlock is invisible at the +// call site, because this reads like a plain getter. Dropping the lock +// deletes the edge rather than documenting it. +// +// What that gives up: a caller racing an in-flight EndSync can observe +// the token being finalized instead of blocking until the sync detaches. +// The value still belongs to a sync that was bound at a real instant +// during the call — the answer a caller a moment earlier would have +// received — and every caller reads this while it owns the sync's +// lifecycle, so none can tell the difference. func (e *Engine) CurrentSyncStep(ctx context.Context) (string, error) { - e.lifecycleMu.Lock() - defer e.lifecycleMu.Unlock() - syncID := e.CurrentSyncID() - if syncID == "" { - return "", nil - } - rec, err := e.GetSyncRunRecord(ctx, syncID) - if err != nil { - if errors.Is(err, pebble.ErrNotFound) { + for pass := 0; ; pass++ { + // Every pass past the first is waiting on transitions this call + // does not control, and the termination argument below assumes + // they stop coming; if they don't, the caller's cancellation is + // the only way out. The first pass stays unguarded so a caller + // reading the step while shutting down — which is when the + // expiry checkpoint reads it — still gets an answer. + if pass > 0 { + if err := ctx.Err(); err != nil { + return "", err + } + } + syncID, gen := e.currentSyncBinding() + if syncID == "" { return "", nil } - return "", err + if e.test.currentSyncStepPreReadHook != nil { + e.test.currentSyncStepPreReadHook() + } + rec, err := e.GetSyncRunRecord(ctx, syncID) + if err != nil { + if !errors.Is(err, pebble.ErrNotFound) { + return "", err + } + // Not-found clears the same bar as a hit, or the two answers + // disagree about which sync they describe. startNewSync bumps + // the generation in MarkFreshSync before it writes the new + // record, so a reader that sampled the previous binding can + // arrive after the swap and be told "no such sync" — about a + // sync that was never unbound, by a lookup that keys on an id + // the engine has already left. Reporting no step there is a + // lie the locked version could not tell. + if _, after := e.currentSyncBinding(); after == gen { + return "", nil + } + continue + } + if _, after := e.currentSyncBinding(); after == gen { + return rec.GetSyncToken(), nil + } + // A transition committed while we were reading; the record we + // have may belong to a sync that is no longer bound, so read + // again against the new binding. This terminates for the same + // reason the lock-holding version made progress: transitions are + // serialized by lifecycleMu and happen a handful of times per + // sync, so spinning here needs an unbounded stream of them — + // which would have starved the blocking version too. The + // cancellation check at the top bounds that case anyway. } - return rec.GetSyncToken(), nil } // CheckpointSync persists a step token to the open sync's record. func (e *Engine) CheckpointSync(ctx context.Context, syncToken string) error { + // Guard BEFORE lifecycleMu; see startNewSync for why the body's own + // barrier take fires too late to catch the deadlock. + e.assertNotTakingLifecycleFromWrite() + if err := e.admit.enterWrite(); err != nil { + return err + } + defer e.admit.exitWrite() e.lifecycleMu.Lock() defer e.lifecycleMu.Unlock() syncID := e.CurrentSyncID() @@ -258,6 +347,18 @@ func (e *Engine) CheckpointSync(ctx context.Context, syncToken string) error { // cleared inside the finalize tail (EndFreshSync), so success leaves // no lifecycle state to reset here. func (e *Engine) EndSync(ctx context.Context) error { + // Guard BEFORE lifecycleMu; see startNewSync for why the finalize's + // own barrier takes fire too late to catch the deadlock. + e.assertNotTakingLifecycleFromWrite() + // Admitted as one write for the whole finalize: Close waits the + // finalize out (or, flipping mid-way, refuses its next nested step, + // which unwinds through EndSync's unseal error path). This is also + // what covers the finalize's bare handle reads — DeferredIdxPending, + // the digest repair scans — against the teardown. + if err := e.admit.enterWrite(); err != nil { + return err + } + defer e.admit.exitWrite() e.lifecycleMu.Lock() defer e.lifecycleMu.Unlock() syncID := e.CurrentSyncID() diff --git a/pkg/dotc1z/engine/pebble/adapter_reader.go b/pkg/dotc1z/engine/pebble/adapter_reader.go index d0782dd56..3687b5c7b 100644 --- a/pkg/dotc1z/engine/pebble/adapter_reader.go +++ b/pkg/dotc1z/engine/pebble/adapter_reader.go @@ -343,7 +343,13 @@ func (e *Engine) entitlementIdentityForRequest(ctx context.Context, ent *v2.Enti if res := ent.GetResource(); res.GetId().GetResourceType() != "" && res.GetId().GetResource() != "" { return entitlementIdentityFromParts(res.GetId().GetResourceType(), res.GetId().GetResource(), ent.GetId()), nil } - return e.resolveGrantScanEntitlementIdentity(ctx, ent.GetId()) + // Only the fallback touches the store, so only it pins. + db, release, err := e.pinRead() + if err != nil { + return entitlementIdentity{}, err + } + defer release() + return e.resolveGrantScanEntitlementIdentity(ctx, db, ent.GetId()) } // ListGrantsForPrincipal returns all grants where the given principal_id is @@ -752,11 +758,16 @@ func (e *Engine) GetEntitlementGrantDigestNodes(ctx context.Context, ent *v2.Ent // At or below the stored width, fold the digest leaves (cheap). Finer // than what we stored, scan the grant index to compute the rollup. partition := digestPartitionForEntitlement(id) + db, release, err := e.pinRead() + if err != nil { + return nil, false, err + } + defer release() var folded []foldedBucket if bits <= root.Bits { - folded, err = e.foldedLeafBuckets(ctx, grantDigestSpec, partition, bits) + folded, err = e.foldedLeafBuckets(ctx, db, grantDigestSpec, partition, bits) } else { - folded, err = e.computeBucketsAtWidth(ctx, grantDigestSpec, partition, bits) + folded, err = e.computeBucketsAtWidth(ctx, db, grantDigestSpec, partition, bits) } if err != nil { return nil, false, err diff --git a/pkg/dotc1z/engine/pebble/admission.go b/pkg/dotc1z/engine/pebble/admission.go new file mode 100644 index 000000000..ae2aa25f3 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/admission.go @@ -0,0 +1,263 @@ +package pebble + +import ( + "sync" + "sync/atomic" +) + +// Panic messages for waiting on yourself through the drains below. +// Constants so the regression tests assert the exact value rather than +// a substring that could drift. +const ( + writeBarrierWaitFromWritePanic = "pebble engine: waited for in-flight writes from inside a write body — " + + "this goroutine's own write is one of them, so the wait can never finish." + readPinWaitFromReadPanic = "pebble engine: waited for in-flight reads from inside a pinned read — this " + + "goroutine's own read is one of them, so the wait can never finish. Iterate* and ForEach* hold the pin " + + "across the yield callback, so closing the engine from inside one waits on itself." + admissionUnderflowPanic = "pebble engine: admission exit without a matching enter — an operation released " + + "the gate twice, so the drain accounting is corrupt." +) + +// admission is the engine's open/close gate. Operations enter as reads +// or writes and exit when done; Close flips the gate shut and waits for +// every operation already inside; anything arriving after the flip gets +// ErrEngineClosing. It answers exactly one question — "may this +// operation borrow the handle Close tears down?" — and owns everything +// needed to answer it, so the invariants live behind this type's five +// methods instead of on the Engine struct. +// +// The load-bearing detail is that entering is atomic against the flip. +// A closing check and a counter increment are two steps, and re-checking +// after the increment does not make them one: a joiner that read +// closing==false can be descheduled and land its increment after close +// has started draining at zero, becoming a member the drain never +// counted. So joiners hold mu.RLock across the check and the increment, +// and closeAndDrain holds mu.Lock across the flip: the increment either +// precedes the flip (close waits for that operation) or never happens. +// +// The members are counted with plain integers and a condition variable, +// NOT sync.WaitGroup. WaitGroup's contract forbids an Add concurrent +// with a Wait that started at counter zero — a rule closeAndDrain can +// honor (the flip stops new Adds before the Wait), but drainWrites +// cannot: it drains while the gate stays OPEN, so a new writer entering +// in the instant the counter touches zero is legal here and answered by +// WaitGroup with the "Add called concurrently with Wait" runtime fatal. +// A cond-based wait-until-zero admits joiners while parked and simply +// keeps waiting, which is the semantic both drains actually want. +// +// The drains also refuse to wait on their own caller. Both waits are +// forever when the calling goroutine is a member of the group being +// drained — the read side is the invited shape, since a pinned read runs +// caller-supplied code (an Iterate*/ForEach* yield callback) for the +// whole of the pin — so closeAndDrain and drainWrites check membership +// first and panic with the constants above instead of hanging silently. +// Membership tracking costs a runtime-stack format per operation and is +// compiled in only when writeBarrierOwnerChecks is set +// (lock_checks_enabled.go); unchecked builds skip the check and would +// hang, which is why the armed builds are the ones CI runs. +type admission struct { + // mu makes entering atomic against closeAndDrain's flip; see the + // type comment. Joiners RLock, the flip Locks. + mu sync.RWMutex + closing atomic.Bool + // closeMu serializes closeAndDrain bodies; closed (guarded by it) + // makes the teardown run exactly once, with later calls returning + // nil. + closeMu sync.Mutex + closed bool + // countMu guards writers/readers; zero (lazily minted on countMu) + // is broadcast whenever either counter returns to zero, waking the + // drains to re-check their condition. + countMu sync.Mutex + zero *sync.Cond + writers int + readers int + // Membership of the two groups by goroutine id, for the self-wait + // panics. Empty (and never written) in unchecked builds. + writerIDs goroutineSet + readerIDs goroutineSet +} + +// zeroCond returns the drain wake-up cond, minting it on first use so +// the zero value of admission stays usable. Callers hold countMu. +func (a *admission) zeroCond() *sync.Cond { + if a.zero == nil { + a.zero = sync.NewCond(&a.countMu) + } + return a.zero +} + +// enterWrite admits the caller as a write, or refuses with +// ErrEngineClosing once the gate is shut. On success the caller must +// exitWrite when done, normally with an immediate defer. Entering is +// counted, not owned: a goroutine already admitted may enter again (a +// nested withWrite inside an admitted lifecycle transition), and the +// drains wait for every entry to exit. +func (a *admission) enterWrite() error { + a.mu.RLock() + closing := a.closing.Load() + if !closing { + a.countMu.Lock() + a.writers++ + a.countMu.Unlock() + if self := trackedGoroutineID(); self != 0 { + a.writerIDs.enter(self) + } + } + a.mu.RUnlock() + if closing { + return ErrEngineClosing + } + return nil +} + +// exitWrite releases an enterWrite. +func (a *admission) exitWrite() { + if self := trackedGoroutineID(); self != 0 { + a.writerIDs.exit(self) + } + a.countMu.Lock() + a.writers-- + if a.writers < 0 { + a.countMu.Unlock() + panic(admissionUnderflowPanic) + } + if a.writers == 0 { + a.zeroCond().Broadcast() + } + a.countMu.Unlock() +} + +// enterRead admits the caller as a read; same contract as enterWrite. +func (a *admission) enterRead() error { + a.mu.RLock() + closing := a.closing.Load() + if !closing { + a.countMu.Lock() + a.readers++ + a.countMu.Unlock() + if self := trackedGoroutineID(); self != 0 { + a.readerIDs.enter(self) + } + } + a.mu.RUnlock() + if closing { + return ErrEngineClosing + } + return nil +} + +// exitRead releases an enterRead. +func (a *admission) exitRead() { + if self := trackedGoroutineID(); self != 0 { + a.readerIDs.exit(self) + } + a.countMu.Lock() + a.readers-- + if a.readers < 0 { + a.countMu.Unlock() + panic(admissionUnderflowPanic) + } + if a.readers == 0 { + a.zeroCond().Broadcast() + } + a.countMu.Unlock() +} + +// isClosing reports whether the gate has been flipped shut. Advisory +// for fail-fast checks: the answer can change immediately after, and +// only enterRead/enterWrite decide admission. +func (a *admission) isClosing() bool { + return a.closing.Load() +} + +// drainWrites waits for every admitted write to exit, without shutting +// the gate — new writes may enter while it waits and after it returns, +// each extending the wait (see the type comment for why that rules out +// sync.WaitGroup). CheckpointTo's quiesce. Panics rather than +// deadlocking when called from inside a write, in armed builds; an +// unchecked build hangs. +func (a *admission) drainWrites() { + if self := trackedGoroutineID(); self != 0 && a.writerIDs.holds(self) { + panic(writeBarrierWaitFromWritePanic) + } + a.countMu.Lock() + for a.writers > 0 { + a.zeroCond().Wait() + } + a.countMu.Unlock() +} + +// closeAndDrain shuts the gate, waits for every admitted operation to +// exit, then runs teardown exactly once, returning its error. Later +// calls return nil after the first completes. Panics rather than +// deadlocking when called from inside an admitted operation (armed +// builds only) — checked before closeMu, not merely before the flip, +// because a concurrent closeAndDrain can already hold the lock and be +// parked in a wait this caller's own operation is keeping parked: +// behind the lock this caller would block silently one lock earlier +// than the check was looking. Asking first also leaves a usable engine +// behind — nothing is marked closing when it panics. +func (a *admission) closeAndDrain(teardown func() error) error { + if self := trackedGoroutineID(); self != 0 { + if a.writerIDs.holds(self) { + panic(writeBarrierWaitFromWritePanic) + } + if a.readerIDs.holds(self) { + panic(readPinWaitFromReadPanic) + } + } + a.closeMu.Lock() + defer a.closeMu.Unlock() + if a.closed { + return nil + } + a.mu.Lock() + a.closing.Store(true) + a.mu.Unlock() + a.countMu.Lock() + for a.writers > 0 || a.readers > 0 { + a.zeroCond().Wait() + } + a.countMu.Unlock() + err := teardown() + a.closed = true + return err +} + +// goroutineSet records which goroutines are currently admitted through +// one side of the gate. +// +// Counted rather than a plain set: nothing forbids a goroutine from +// entering twice — a compaction inside a write body, a second pinned +// read taken inside a scan's yield callback — and a plain delete on the +// inner exit would hide the outer one. +type goroutineSet struct { + mu sync.Mutex + m map[uint64]int +} + +func (s *goroutineSet) enter(self uint64) { + s.mu.Lock() + defer s.mu.Unlock() + if s.m == nil { + s.m = make(map[uint64]int, 1) + } + s.m[self]++ +} + +func (s *goroutineSet) exit(self uint64) { + s.mu.Lock() + defer s.mu.Unlock() + if n := s.m[self]; n > 1 { + s.m[self] = n - 1 + return + } + delete(s.m, self) +} + +func (s *goroutineSet) holds(self uint64) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.m[self] > 0 +} diff --git a/pkg/dotc1z/engine/pebble/admission_test.go b/pkg/dotc1z/engine/pebble/admission_test.go new file mode 100644 index 000000000..1f6fe1ac8 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/admission_test.go @@ -0,0 +1,186 @@ +package pebble + +import ( + "errors" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// These test the admission type directly, with no Engine and no DB +// behind it: the gate's contract is self-contained, and testing it here +// is what lets TestAdmissionUsedOnlyThroughItsMethods confine the +// mechanism to admission.go instead of enumerating WaitGroup call sites +// across the package. + +func TestAdmissionRefusesAfterClose(t *testing.T) { + var a admission + require.NoError(t, a.enterWrite()) + a.exitWrite() + require.NoError(t, a.enterRead()) + a.exitRead() + + require.NoError(t, a.closeAndDrain(func() error { return nil })) + require.True(t, a.isClosing()) + require.ErrorIs(t, a.enterWrite(), ErrEngineClosing) + require.ErrorIs(t, a.enterRead(), ErrEngineClosing) +} + +func TestAdmissionCloseWaitsForAdmittedOperations(t *testing.T) { + var a admission + require.NoError(t, a.enterWrite()) + require.NoError(t, a.enterRead()) + + var toreDown atomic.Bool + closed := make(chan error, 1) + go func() { + closed <- a.closeAndDrain(func() error { + toreDown.Store(true) + return nil + }) + }() + + // The gate must shut (new arrivals refused) while it drains, but the + // teardown must not run until both members exit. + require.Eventually(t, a.isClosing, 10*time.Second, time.Millisecond) + require.ErrorIs(t, a.enterWrite(), ErrEngineClosing) + require.Never(t, toreDown.Load, 100*time.Millisecond, 5*time.Millisecond, + "teardown ran while operations were still admitted") + + a.exitWrite() + require.Never(t, toreDown.Load, 100*time.Millisecond, 5*time.Millisecond, + "teardown ran while a read was still admitted") + a.exitRead() + require.NoError(t, <-closed) + require.True(t, toreDown.Load()) +} + +func TestAdmissionTeardownRunsExactlyOnce(t *testing.T) { + var a admission + calls := 0 + sentinel := errors.New("teardown result") + require.ErrorIs(t, a.closeAndDrain(func() error { calls++; return sentinel }), sentinel) + // Later closes are nil, matching Engine.Close's idempotency: the + // engine is closed, there is nothing left to report. + require.NoError(t, a.closeAndDrain(func() error { calls++; return nil })) + require.Equal(t, 1, calls) +} + +func TestAdmissionDrainWritesQuiescesWithoutShutting(t *testing.T) { + var a admission + require.NoError(t, a.enterWrite()) + + drained := make(chan struct{}) + go func() { + a.drainWrites() + close(drained) + }() + select { + case <-drained: + t.Fatal("drainWrites returned while a write was admitted") + case <-time.After(100 * time.Millisecond): + } + a.exitWrite() + <-drained + + // Not a close: both sides stay open. + require.False(t, a.isClosing()) + require.NoError(t, a.enterWrite()) + a.exitWrite() +} + +// TestAdmissionEnterNeverTripsDrainingWaitGroup hammers the exact +// interleaving the gate's mu exists for: enters racing a close whose +// drain is parked at zero. Without admission atomicity a joiner that +// read closing==false can land its increment after the drain sampled +// zero — a member the drain never counted (and, in the WaitGroup +// implementation this replaced, the "Add called concurrently with Wait" +// runtime fatal — a crash, not a test failure). Every refused enter +// must also be refused with the error, never admitted after the flip. +func TestAdmissionEnterNeverTripsDrainingWaitGroup(t *testing.T) { + for round := 0; round < 200; round++ { + var a admission + var admitted sync.WaitGroup + start := make(chan struct{}) + const workers = 8 + results := make(chan error, workers) + for i := 0; i < workers; i++ { + admitted.Add(1) + go func(i int) { + defer admitted.Done() + <-start + var err error + if i%2 == 0 { + err = a.enterWrite() + if err == nil { + a.exitWrite() + } + } else { + err = a.enterRead() + if err == nil { + a.exitRead() + } + } + results <- err + }(i) + } + closed := make(chan error, 1) + go func() { + <-start + closed <- a.closeAndDrain(func() error { return nil }) + }() + close(start) + admitted.Wait() + require.NoError(t, <-closed) + for i := 0; i < workers; i++ { + if err := <-results; err != nil { + require.ErrorIs(t, err, ErrEngineClosing) + } + } + // After the dust settles the gate must be shut for good. + require.ErrorIs(t, a.enterWrite(), ErrEngineClosing) + } +} + +// TestAdmissionDrainWritesToleratesConcurrentEnters hammers drainWrites +// against a stream of entering writers. This is the drain sync.WaitGroup +// could NOT express: the gate stays open, so a writer legally enters in +// the same instant the counter touches zero — WaitGroup answers that +// with the "Add called concurrently with Wait" runtime fatal, while the +// cond-based drain just keeps waiting. The drain must return only at a +// real zero and the gate must stay open throughout. +func TestAdmissionDrainWritesToleratesConcurrentEnters(t *testing.T) { + for round := 0; round < 200; round++ { + var a admission + const workers = 8 + var churn sync.WaitGroup + start := make(chan struct{}) + for i := 0; i < workers; i++ { + churn.Add(1) + go func() { + defer churn.Done() + <-start + for j := 0; j < 4; j++ { + if err := a.enterWrite(); err == nil { + a.exitWrite() + } + } + }() + } + drained := make(chan struct{}) + go func() { + <-start + a.drainWrites() + close(drained) + }() + close(start) + churn.Wait() + <-drained + require.False(t, a.isClosing(), "drainWrites shut the gate; it must only quiesce") + require.NoError(t, a.enterWrite()) + a.exitWrite() + } +} diff --git a/pkg/dotc1z/engine/pebble/assets.go b/pkg/dotc1z/engine/pebble/assets.go index 327f3f7e3..ab7a13271 100644 --- a/pkg/dotc1z/engine/pebble/assets.go +++ b/pkg/dotc1z/engine/pebble/assets.go @@ -28,7 +28,12 @@ func (e *Engine) PutAssetRecord(ctx context.Context, r *v3.AssetRecord) error { } func (e *Engine) GetAssetRecord(ctx context.Context, externalID string) (*v3.AssetRecord, error) { - val, closer, err := e.db.Get(encodeAssetKey(externalID)) + db, release, err := e.pinRead() + if err != nil { + return nil, err + } + defer release() + val, closer, err := db.Get(encodeAssetKey(externalID)) if err != nil { return nil, err } @@ -47,8 +52,13 @@ func (e *Engine) DeleteAssetRecord(ctx context.Context, externalID string) error } func (e *Engine) IterateAssets(ctx context.Context, yield func(*v3.AssetRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() prefix := encodeAssetPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -57,6 +67,9 @@ func (e *Engine) IterateAssets(ctx context.Context, yield func(*v3.AssetRecord) } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } r := &v3.AssetRecord{} if err := unmarshalRecord(iter.Value(), r); err != nil { return fmt.Errorf("iterate assets: %w", err) diff --git a/pkg/dotc1z/engine/pebble/cleanup.go b/pkg/dotc1z/engine/pebble/cleanup.go index 416c29b50..8a9e0c4c3 100644 --- a/pkg/dotc1z/engine/pebble/cleanup.go +++ b/pkg/dotc1z/engine/pebble/cleanup.go @@ -127,12 +127,12 @@ func (e *Engine) ResetForNewSync(ctx context.Context) error { // Refuses with ErrEngineSealed after EndSync (via checkWritable): manual // compactions go through the same CompactionScheduler as automatic ones, // so on a sealed (paused) engine db.Compact would block forever waiting -// for a grant — and, because we hold writeWG, deadlock Engine.Close too. -// Bind a sync (SetCurrentSync) first. +// for a grant — and, because we sit admitted in the close gate, deadlock +// Engine.Close too. Bind a sync (SetCurrentSync) first. // -// KNOWN LIMITATION: the gate only refuses calls made after the seal. A +// KNOWN LIMITATION: the seal only refuses calls made after it. A // CompactAllRanges already inside its loop when EndSync pauses the -// scheduler blocks in db.Compact indefinitely (and holds writeWG, so a +// scheduler blocks in db.Compact indefinitely (and stays admitted, so a // later Close hangs too). Do not run this concurrently with EndSync; no // in-tree caller does. func (e *Engine) CompactAllRanges(ctx context.Context) error { @@ -142,20 +142,19 @@ func (e *Engine) CompactAllRanges(ctx context.Context) error { if err := e.checkWritable(); err != nil { return err } - // Hold the engine's writeWG for the duration of the compaction - // so Engine.Close blocks until our in-flight Compact returns. - // Without this guard, Close → e.db.Close() can race with our + // Enter the gate as a write for the duration of the compaction so + // Engine.Close blocks until our in-flight Compact returns. Without + // this guard, Close → e.db.Close() can race with our // e.db.Compact(...) call, and pebble.DB.Compact PANICS on a // closed DB (vendor/.../pebble/v2/db.go:1826) rather than // returning an error. Compact doesn't need writeMu — pebble's // own compaction is concurrency-safe with foreground writes, // so we don't go through withWrite (which serializes against // other Puts and DeleteRanges). - e.writeWG.Add(1) - defer e.writeWG.Done() - if e.closing.Load() { - return ErrEngineClosing + if err := e.admit.enterWrite(); err != nil { + return err } + defer e.admit.exitWrite() var firstErr error for _, r := range scopedRanges() { @@ -197,15 +196,14 @@ func (e *Engine) Flush(ctx context.Context) error { if err := e.checkWritable(); err != nil { return err } - // Hold writeWG so Engine.Close blocks until the Flush + WAL - // fsync finish — same close-race protection as CompactSyncRanges. + // Enter the gate as a write so Engine.Close blocks until the Flush + + // WAL fsync finish — same close-race protection as CompactAllRanges. // pebble.DB.Flush and pebble.DB.LogData both panic on a closed // DB rather than returning an error. - e.writeWG.Add(1) - defer e.writeWG.Done() - if e.closing.Load() { - return ErrEngineClosing + if err := e.admit.enterWrite(); err != nil { + return err } + defer e.admit.exitWrite() if err := e.db.FlushMemtables(); err != nil { return fmt.Errorf("engine: flush: %w", err) } diff --git a/pkg/dotc1z/engine/pebble/current_sync_step_lifecycle_test.go b/pkg/dotc1z/engine/pebble/current_sync_step_lifecycle_test.go new file mode 100644 index 000000000..4fd2a0bc2 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/current_sync_step_lifecycle_test.go @@ -0,0 +1,346 @@ +package pebble + +import ( + "context" + "testing" + "time" + + "github.com/segmentio/ksuid" + "github.com/stretchr/testify/require" + + "github.com/conductorone/baton-sdk/pkg/connectorstore" +) + +// lifecycleTestTimeout is how long these tests wait before calling a +// deadlock a deadlock. Both sides do a few key reads, so any real run +// finishes in milliseconds; the budget is loose so a slow CI box does not +// have to be fast to be correct. +const lifecycleTestTimeout = 30 * time.Second + +// readStepInsideWrite is the shape that closed the lock cycle: a write +// that consults the current step while it holds the write barrier. It +// parks between the two so the test can line up the interleaving instead +// of hoping for it. +func (e *Engine) readStepInsideWrite(ctx context.Context, held, proceed chan struct{}) (string, error) { + var step string + err := e.withWrite(func() error { + close(held) + <-proceed + var err error + step, err = e.CurrentSyncStep(ctx) + return err + }) + return step, err +} + +// TestCurrentSyncStepDoesNotDeadlockWithEndSync is the regression test +// for the ABBA deadlock between the write barrier and the lifecycle +// mutex. +// +// The interleaving: a writer holds writeMu and then wants the current +// step, while EndSync holds lifecycleMu and then wants writeMu for its +// finalize. When CurrentSyncStep took lifecycleMu, both sides held what +// the other needed and the process hung — including any goroutine that +// later touched either lock. Neither side is doing anything exotic: one +// is a write that reads its own progress, the other is the ordinary end +// of a sync. +func TestCurrentSyncStepDoesNotDeadlockWithEndSync(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + _, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, e.CheckpointSync(ctx, "mid-sync")) + + held := make(chan struct{}) + proceed := make(chan struct{}) + type writeResult struct { + step string + err error + } + writerDone := make(chan writeResult, 1) + go func() { + step, err := e.readStepInsideWrite(ctx, held, proceed) + writerDone <- writeResult{step: step, err: err} + }() + + // Wait until the writer owns the barrier, so EndSync is guaranteed to + // block on it rather than racing ahead and finishing first — a pass + // that skipped the interleaving would prove nothing. + select { + case <-held: + case <-time.After(lifecycleTestTimeout): + t.Fatal("the writer never took the write barrier") + } + + endDone := make(chan error, 1) + go func() { endDone <- e.EndSync(ctx) }() + requireLifecycleMuHeld(t, e) + + // Both sides are now holding one lock and about to want the other. + close(proceed) + + select { + case got := <-writerDone: + require.NoError(t, got.err) + require.Equal(t, "mid-sync", got.step, "the step read inside the write must be the bound sync's") + case <-time.After(lifecycleTestTimeout): + t.Fatal("the write blocked reading the current step while EndSync held the lifecycle mutex") + } + select { + case err := <-endDone: + require.NoError(t, err) + case <-time.After(lifecycleTestTimeout): + t.Fatal("EndSync never completed after the write released the barrier") + } + + // EndSync detached the sync, so there is no step to report. + step, err := e.CurrentSyncStep(ctx) + require.NoError(t, err) + require.Empty(t, step) +} + +// TestCurrentSyncStepReadsRebindFromTheRecord covers the durable read: +// after a rebind the step comes from that sync's record, with no +// in-memory step cache to go stale, and the generation the retry loop +// samples actually moves on a transition. It is deliberately sequential, +// so every call sees an unchanged generation and returns on the first +// pass — the retry branch is exercised by +// TestCurrentSyncStepRetriesWhenBindingMovesMidRead below. +func TestCurrentSyncStepReadsRebindFromTheRecord(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + first, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, e.CheckpointSync(ctx, "first-token")) + require.NoError(t, e.EndSync(ctx)) + + // Rebinding the finished sync is what SetCurrentSync is for; the step + // must come from that sync's record and not from anything cached. + require.NoError(t, e.SetCurrentSync(ctx, first)) + step, err := e.CurrentSyncStep(ctx) + require.NoError(t, err) + require.Equal(t, "first-token", step) + + // A generation that never moved would make the retry loop vacuous, so + // check the counter actually advances with the transitions above. + _, gen := e.currentSyncBinding() + require.Greater(t, gen, uint64(0), "binding transitions must bump the generation") + e.clearCurrentSync() + _, cleared := e.currentSyncBinding() + require.Greater(t, cleared, gen, "clearing the binding must bump the generation") +} + +// TestCurrentSyncStepRetriesWhenBindingMovesMidRead executes the retry +// branch. The window it protects is two statements wide — sample the +// generation, read the record — so a transition has to land inside +// another goroutine's read to reach it, which no amount of concurrent +// hammering can be made to guarantee. The seam puts the transition +// there. +// +// The oracle is the answer, not the retry count: a read that returned +// after the first pass would report the step of the sync that was bound +// when it started, which is the inconsistency dropping lifecycleMu had to +// pay for somewhere. +func TestCurrentSyncStepRetriesWhenBindingMovesMidRead(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + _, other := boundSyncAndSpareID(t, e) + + // Rebind on the first pass only. The hook runs on every iteration, so + // a hook that rebinds every time is an infinite retry. + passes := 0 + e.test.currentSyncStepPreReadHook = func() { + passes++ + if passes > 1 { + return + } + require.NoError(t, e.SetCurrentSync(ctx, other)) + } + + step, err := e.CurrentSyncStep(ctx) + require.NoError(t, err) + require.Equal(t, 2, passes, "the binding moved inside the read window, so the read must have been retried") + require.Empty(t, step, + `the retry must answer for the binding in force when it finished; "token-a" is the sync that was bound when it started`) +} + +// TestCurrentSyncStepRetriesWhenBindingMovesMidReadOnMiss is the same +// window, reached through the not-found branch instead of the hit. +// +// A miss is an answer too, and it has to clear the same bar: the record +// lookup keys on the id sampled at the top, so once the binding has +// moved, "no record for that id" says nothing about the sync now bound. +// Returning "" there reports no step for a sync that never unbound and +// has one — the inconsistency the lock used to rule out, arrived at by +// the one path that skipped the generation re-check. +func TestCurrentSyncStepRetriesWhenBindingMovesMidReadOnMiss(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + bound, spare := boundSyncAndSpareID(t, e) + + // Start from the binding with no record, so the first pass misses. + require.NoError(t, e.SetCurrentSync(ctx, spare)) + + passes := 0 + e.test.currentSyncStepPreReadHook = func() { + passes++ + if passes > 1 { + return + } + require.NoError(t, e.SetCurrentSync(ctx, bound)) + } + + step, err := e.CurrentSyncStep(ctx) + require.NoError(t, err) + require.Equal(t, 2, passes, "the binding moved inside the read window, so the miss must have been retried") + require.Equal(t, "token-a", step, + `the retry must answer for the binding in force when it finished; "" is the spare that was bound when it started`) +} + +// TestCurrentSyncStepRetryHonorsCancellation drives the retry branch +// forever and requires the call to come back anyway. A hook that rebinds +// on every pass is the pathological case the loop's termination argument +// sets aside as unreachable — transitions are supposed to run out — and +// the argument is about the engine's own behavior, so nothing in it +// protects a caller from a bug or a hostile workload that keeps them +// coming. Without the cancellation check this hangs until the test +// binary's timeout rather than failing. +func TestCurrentSyncStepRetryHonorsCancellation(t *testing.T) { + e, _ := newTestEngine(t) + bound, spare := boundSyncAndSpareID(t, e) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Rebind to a different id on every pass, so the generation re-check + // never agrees and the read can only end by giving up. Cancel from + // inside the loop rather than up front: the first pass is deliberately + // unguarded, so a context that was already dead would prove nothing + // about the retry. + // + // The hook runs on the goroutine below, where require's FailNow is + // not legal, so failures are carried back and asserted here. + setup := context.Background() + ids := []string{bound, spare} + passes := 0 + var rebindErr error + e.test.currentSyncStepPreReadHook = func() { + passes++ + if err := e.SetCurrentSync(setup, ids[passes%len(ids)]); err != nil && rebindErr == nil { + rebindErr = err + } + if passes == 2 { + cancel() + } + } + + done := make(chan error, 1) + go func() { + _, err := e.CurrentSyncStep(ctx) + done <- err + }() + + // Reading passes and rebindErr is only safe on this side of the + // channel, which is also why the timeout branch does not. + select { + case err := <-done: + require.NoError(t, rebindErr, "rebinding the sync inside the read window failed") + require.ErrorIs(t, err, context.Canceled, + "a retry loop that outlives its caller's context has no way to stop") + require.Greater(t, passes, 1, "the read must have reached the retry branch, not just the first pass") + case <-time.After(lifecycleTestTimeout): + t.Fatal("CurrentSyncStep kept retrying after its context was cancelled") + } +} + +// boundSyncAndSpareID starts a sync with the step token "token-a" and +// returns its id plus a second id that is bindable but has no record of +// its own. +// +// The spare has no record because it cannot: a v3 Pebble c1z holds +// exactly one sync and its record lives at a single fixed key, so a +// second StartNewSync wipes the first (ResetForNewSync) and a second +// PutSyncRunRecord overwrites it. SetCurrentSync deliberately allows a +// binding whose record is absent — GetSyncRunRecord's miss is not an +// error to it — and CurrentSyncStep answers "" for that state, which is +// what makes the spare a usable second binding. +func boundSyncAndSpareID(t *testing.T, e *Engine) (string, string) { + t.Helper() + ctx := context.Background() + bound, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, e.CheckpointSync(ctx, "token-a")) + + step, err := e.CurrentSyncStep(ctx) + require.NoError(t, err) + require.Equal(t, "token-a", step, "the bound sync must report its own token before anything moves") + + // A KSUID, because that is what the key codec accepts. + return bound, ksuid.New().String() +} + +// TestCurrentSyncStepUnderConcurrentTransitions is the -race soak on the +// lock-free read. The seam test above pins the retry deterministically +// but says nothing about the unsynchronized field access underneath it, +// which is what the race detector is for. The oracle is weak on purpose: +// with transitions landing at arbitrary points, the only invariant left +// is that every answer belongs to a sync that was bound at some point +// during the call — the bound sync's own token, or "" for a binding with +// no record — never a torn or invented one. +func TestCurrentSyncStepUnderConcurrentTransitions(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + + bound, spare := boundSyncAndSpareID(t, e) + ids := []string{bound, spare} + + const transitions = 300 + done := make(chan error, 1) + go func() { + for i := 0; i < transitions; i++ { + if err := e.SetCurrentSync(ctx, ids[i%len(ids)]); err != nil { + done <- err + return + } + if i%3 == 0 { + e.clearCurrentSync() + } + } + done <- nil + }() + + // Read on this goroutine: require's FailNow is only legal here. + for { + select { + case err := <-done: + require.NoError(t, err) + return + default: + } + step, err := e.CurrentSyncStep(ctx) + require.NoError(t, err) + if step == "" { + continue // cleared, or bound to the spare + } + require.Equal(t, "token-a", step, + "CurrentSyncStep returned a token belonging to no bound sync") + } +} + +// requireLifecycleMuHeld waits until some other goroutine owns +// lifecycleMu. TryLock is the only way to ask, and this test needs the +// answer: without it, EndSync might not have reached its lock yet when +// the writer is released, and the ordering the test exists to check +// would not have happened. +func requireLifecycleMuHeld(t *testing.T, e *Engine) { + t.Helper() + deadline := time.Now().Add(lifecycleTestTimeout) + for time.Now().Before(deadline) { + if !e.lifecycleMu.TryLock() { + return + } + e.lifecycleMu.Unlock() + time.Sleep(time.Millisecond) + } + t.Fatal("EndSync never took the lifecycle mutex") +} diff --git a/pkg/dotc1z/engine/pebble/deferred_index.go b/pkg/dotc1z/engine/pebble/deferred_index.go index 4baad1691..e4fe5fdf3 100644 --- a/pkg/dotc1z/engine/pebble/deferred_index.go +++ b/pkg/dotc1z/engine/pebble/deferred_index.go @@ -268,7 +268,7 @@ func (t *grantRebuildTee) closeAndWait() { // would be silently erased. EndSync's callers are expected to have quiesced // writers already — holding writeMu for the duration converts that // convention into an enforced invariant (a straggler write blocks until the -// build finishes instead of racing the excise), and writeWG participation +// build finishes instead of racing the excise), and gate participation // means Close waits the build out instead of tearing down e.db under it. func (e *Engine) BuildDeferredGrantIndexes(ctx context.Context) error { // AllowSealed: EndSync seals BEFORE running this build so no straggler diff --git a/pkg/dotc1z/engine/pebble/digest.go b/pkg/dotc1z/engine/pebble/digest.go index a37ff721f..2590cf996 100644 --- a/pkg/dotc1z/engine/pebble/digest.go +++ b/pkg/dotc1z/engine/pebble/digest.go @@ -480,7 +480,10 @@ type DigestRoot struct { // nodes, so with no root the index range is absent too and // computeBucketDigest would read that absence as "zero records" — the // false-clean trap dirtyPartitionBuckets' doc comment describes. -func (e *Engine) getPartitionDigestRoot(spec digestIndexSpec, partition string) (DigestRoot, bool, error) { +// db is the caller's admitted handle (pinned read or admitted write); +// the whole digest read surface threads it so one admission at the +// entry point covers every probe and fold below it. +func (e *Engine) getPartitionDigestRoot(db *rawdb.DB, spec digestIndexSpec, partition string) (DigestRoot, bool, error) { if e.grantDigestBuildPending.Load() { // An interrupted digest build's half-committed nodes may be // durable while its hash index never ingested; until the pending @@ -489,7 +492,7 @@ func (e *Engine) getPartitionDigestRoot(spec digestIndexSpec, partition string) // which every consumer already treats as "recalculate". return DigestRoot{}, false, nil } - val, closer, err := e.db.Get(encodeDigestNodeKey(spec.indexID, partition, digestLevelRoot, nil)) + val, closer, err := db.Get(encodeDigestNodeKey(spec.indexID, partition, digestLevelRoot, nil)) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return DigestRoot{}, false, nil @@ -508,8 +511,8 @@ func (e *Engine) getPartitionDigestRoot(spec digestIndexSpec, partition string) // getDigestLeaf reads one stored leaf by its key prefix. An absent leaf // returns (0, zero digest, present=false, nil) — the XOR identity. -func (e *Engine) getDigestLeaf(spec digestIndexSpec, partition string, leafPrefix []byte) (int64, []byte, bool, error) { - val, closer, err := e.db.Get(encodeDigestNodeKey(spec.indexID, partition, digestLevelLeaf, leafPrefix)) +func (e *Engine) getDigestLeaf(db *rawdb.DB, spec digestIndexSpec, partition string, leafPrefix []byte) (int64, []byte, bool, error) { + val, closer, err := db.Get(encodeDigestNodeKey(spec.indexID, partition, digestLevelLeaf, leafPrefix)) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return 0, zeroDigest[:], false, nil @@ -541,9 +544,9 @@ type foldedBucket struct { // range scan; folding is exact because leaf prefixes are left-aligned // (so keys sort in bucket-hash order at any width) and XOR digests are // split-independent. -func (e *Engine) foldedLeafBuckets(ctx context.Context, spec digestIndexSpec, partition string, foldBits int) ([]foldedBucket, error) { +func (e *Engine) foldedLeafBuckets(ctx context.Context, db *rawdb.DB, spec digestIndexSpec, partition string, foldBits int) ([]foldedBucket, error) { stem := encodeDigestNodeKey(spec.indexID, partition, digestLevelLeaf, nil) - iter, err := e.db.NewIter(&pebble.IterOptions{LowerBound: stem, UpperBound: upperBoundOf(stem)}) + iter, err := db.NewIter(&pebble.IterOptions{LowerBound: stem, UpperBound: upperBoundOf(stem)}) if err != nil { return nil, err } @@ -588,9 +591,9 @@ func (e *Engine) foldedLeafBuckets(ctx context.Context, spec digestIndexSpec, pa // partition the fold reads an absent index range and returns {0, 0}, // indistinguishable from a truly empty partition; it must never be // used as a fallback for a missing root (see getPartitionDigestRoot). -func (e *Engine) computeBucketDigest(ctx context.Context, spec digestIndexSpec, partition string, bucket DigestBucket) ([]byte, int64, error) { +func (e *Engine) computeBucketDigest(ctx context.Context, db *rawdb.DB, spec digestIndexSpec, partition string, bucket DigestBucket) ([]byte, int64, error) { lower, upper := spec.bucketBounds(partition, bucket) - iter, err := e.db.NewIter(&pebble.IterOptions{LowerBound: lower, UpperBound: upper}) + iter, err := db.NewIter(&pebble.IterOptions{LowerBound: lower, UpperBound: upper}) if err != nil { return nil, 0, err } @@ -625,9 +628,9 @@ func (e *Engine) computeBucketDigest(ctx context.Context, spec digestIndexSpec, // to the bucket-hash resolution. Returns the non-empty buckets in index // order — index entries are bucket-hash-major, so each bucket's records // are contiguous and close when the top-`bits` prefix changes. -func (e *Engine) computeBucketsAtWidth(ctx context.Context, spec digestIndexSpec, partition string, bits int) ([]foldedBucket, error) { +func (e *Engine) computeBucketsAtWidth(ctx context.Context, db *rawdb.DB, spec digestIndexSpec, partition string, bits int) ([]foldedBucket, error) { prefix := spec.partitionPrefix(partition) - iter, err := e.db.NewIter(&pebble.IterOptions{LowerBound: prefix, UpperBound: upperBoundOf(prefix)}) + iter, err := db.NewIter(&pebble.IterOptions{LowerBound: prefix, UpperBound: upperBoundOf(prefix)}) if err != nil { return nil, err } @@ -681,11 +684,24 @@ func (e *Engine) computeBucketsAtWidth(ctx context.Context, spec digestIndexSpec // would compare falsely clean). So the whole partition is reported // dirty; the caller re-reads it, or rebuilds the digest first. func (e *Engine) dirtyPartitionBuckets(ctx context.Context, spec digestIndexSpec, other *Engine, partition string) ([]DigestBucket, error) { - rootA, okA, err := e.getPartitionDigestRoot(spec, partition) + // Two engines, two gates: each side's handle is pinned for the whole + // comparison so neither side's Close can tear down mid-merge. + dbA, releaseA, err := e.pinRead() + if err != nil { + return nil, err + } + defer releaseA() + dbB, releaseB, err := other.pinRead() + if err != nil { + return nil, err + } + defer releaseB() + + rootA, okA, err := e.getPartitionDigestRoot(dbA, spec, partition) if err != nil { return nil, err } - rootB, okB, err := other.getPartitionDigestRoot(spec, partition) + rootB, okB, err := other.getPartitionDigestRoot(dbB, spec, partition) if err != nil { return nil, err } @@ -705,11 +721,11 @@ func (e *Engine) dirtyPartitionBuckets(ctx context.Context, spec digestIndexSpec return []DigestBucket{{}}, nil } - fa, err := e.foldedLeafBuckets(ctx, spec, partition, compareBits) + fa, err := e.foldedLeafBuckets(ctx, dbA, spec, partition, compareBits) if err != nil { return nil, err } - fb, err := other.foldedLeafBuckets(ctx, spec, partition, compareBits) + fb, err := other.foldedLeafBuckets(ctx, dbB, spec, partition, compareBits) if err != nil { return nil, err } diff --git a/pkg/dotc1z/engine/pebble/digest_test.go b/pkg/dotc1z/engine/pebble/digest_test.go index 837e712e8..c6ba4fd93 100644 --- a/pkg/dotc1z/engine/pebble/digest_test.go +++ b/pkg/dotc1z/engine/pebble/digest_test.go @@ -1074,7 +1074,8 @@ func TestDigestLeafFoldConsistent(t *testing.T) { } // Folding at the build width returns the stored leaves one-to-one. - leaves, err := e.foldedLeafBuckets(ctx, grantDigestSpec, partition, 8) + // (White-box: e.db is stable here — no concurrent Close in this test.) + leaves, err := e.foldedLeafBuckets(ctx, e.db, grantDigestSpec, partition, 8) if err != nil { t.Fatal(err) } @@ -1103,7 +1104,7 @@ func TestDigestLeafFoldConsistent(t *testing.T) { // Folding to a coarser width matches a manual regroup of the // build-width leaves. - leaves4, err := e.foldedLeafBuckets(ctx, grantDigestSpec, partition, 4) + leaves4, err := e.foldedLeafBuckets(ctx, e.db, grantDigestSpec, partition, 4) if err != nil { t.Fatal(err) } @@ -1137,7 +1138,7 @@ func TestDigestLeafFoldConsistent(t *testing.T) { if err != nil { t.Fatal(err) } - lc, ld, present, err := e.getDigestLeaf(grantDigestSpec, partition, b.leafKeyPrefix()) + lc, ld, present, err := e.getDigestLeaf(e.db, grantDigestSpec, partition, b.leafKeyPrefix()) if err != nil || !present { t.Fatalf("leaf %d: present=%v err=%v", b.Index, present, err) } diff --git a/pkg/dotc1z/engine/pebble/engine.go b/pkg/dotc1z/engine/pebble/engine.go index 6e702cf08..41b534887 100644 --- a/pkg/dotc1z/engine/pebble/engine.go +++ b/pkg/dotc1z/engine/pebble/engine.go @@ -60,6 +60,13 @@ type Engine struct { // whose bodies are read-check-write sequences over the sync-run // record + the currentSync binding. Formerly the Adapter layer's // mutex; the record writes themselves ride the write barrier. + // + // Lock order: lifecycleMu, then writeMu. EndSync holds this across a + // finalize whose steps take the barrier, so anything already holding + // the barrier must not take this. CurrentSyncStep used to, which + // closed the cycle from a method that reads like a plain getter; + // TestLifecycleMuTakersAreTransitionsOnly keeps the taker set down to + // the five transitions so acquiring it stays a deliberate act. lifecycleMu sync.Mutex // resolvedFS is rawdb's Open-time FS resolution (WithVFS override // or vfs.Default), snapshotted so fs() stays valid after Close @@ -72,6 +79,12 @@ type Engine struct { // they return ErrNoCurrentSync. currentSyncMu sync.RWMutex currentSync []byte + // currentSyncGen counts binding transitions: every bind, fresh-sync + // bind, and clear bumps it. A reader that samples it on both sides of + // a record read can tell whether the binding moved underneath it, + // which is how CurrentSyncStep gets a consistent answer without + // taking lifecycleMu. + currentSyncGen uint64 // freshSync is true between MarkFreshSync (called by StartNewSync) // and EndSync. Indicates the engine can take perf shortcuts that // trade durability for throughput while the connector is the @@ -91,12 +104,22 @@ type Engine struct { freshGrantsEmpty bool freshResourcesEmpty bool - // writeWG tracks in-flight writes. Incremented at the start of - // every Writer method, decremented in defer. - writeWG sync.WaitGroup + // admit is the open/close gate: reads and writes enter it for their + // duration, Close shuts it and waits for everyone inside, and + // late arrivals get ErrEngineClosing. The atomicity of entering + // against the shut, the exactly-once teardown, and the self-wait + // panics all live behind its methods — see the admission type + // (admission.go) for the invariants. + admit admission + // writeMu is the write barrier: it serializes write bodies against + // each other and against CheckpointTo's Flush→Checkpoint window and + // Close's teardown. Distinct from admission — CompactAllRanges and + // Flush enter the gate as writes without ever taking the barrier. writeMu sync.Mutex - closing atomic.Bool // strict write-barrier flag, read on every Writer call - closeMu sync.Mutex + // writeBarrierOwner is the id of the goroutine holding writeMu, or 0 + // when it is unheld. Recorded only in checked builds; see + // lockWriteBarrier for why the bookkeeping is gated. + writeBarrierOwner atomic.Uint64 // computedStats holds caller-computed stats records stashed via // StashComputedSyncStats, keyed by sync_id. PersistSyncStats pops @@ -316,49 +339,55 @@ func Open(ctx context.Context, dir string, opts ...Option) (*Engine, error) { } // Close shuts down the engine. After Close, all methods return -// ErrEngineClosing. Close blocks until all in-flight writes complete. +// ErrEngineClosing. Close blocks until all in-flight writes and all +// pinned reads (see pinRead) complete, so a slow page read delays it. func (e *Engine) Close() error { - e.closeMu.Lock() - defer e.closeMu.Unlock() - if e.db == nil { - return nil - } - e.closing.Store(true) - e.writeWG.Wait() - // A leaked synthesized-grant layer session (possible only if a panic - // unwound past the expansion driver's Abort) has a background worker - // ingesting through e.db; drain it before tearing the DB down. This - // runs AFTER the closing/writeWG barrier so no in-flight Add/Finish - // (which run under withWrite) can be touching the session concurrently - // — Abort itself takes no write barrier, only synthLayerMu for the - // pointer handoff, and is a no-op when no session is open. - _ = e.AbortSynthesizedGrantLayer(context.Background()) - // Hold writeMu for the teardown: writeWG only covers withWrite users, - // while CheckpointTo takes writeMu directly (no WG participation). A - // CheckpointTo that passed its closing check but hasn't locked yet must - // find either the mutex held or db nil'd under the lock — never a db - // torn down mid-checkpoint. - e.writeMu.Lock() - defer e.writeMu.Unlock() - // Invariant: flush before close on any write path. This drives the - // memtable out to an SST so a Close is never the step that leaves - // un-materialized writes behind — independent of whether EndSync or - // CheckpointTo (which flush for their own reasons) ran first. Skipped - // in read-only mode, where Flush is illegal and there is nothing to - // harden. A no-op when the memtable is already empty. - var err error - if !e.opts.readOnly { - if ferr := e.db.FlushMemtables(); ferr != nil { - err = fmt.Errorf("flush during close: %w", ferr) + // The gate shuts, drains reads and writes (reads borrow the handle + // without any barrier, so draining writes alone would leave a + // paginate holding e.db while the teardown runs — pebble's + // "pebble: closed" panic out of its next iterator call), and runs + // the teardown exactly once. A Close from inside an admitted + // operation panics instead of self-deadlocking in armed builds + // (baton_lockchecks or -race — see writeBarrierOwnerChecks); + // unchecked builds hang, which is why CI runs armed. + return e.admit.closeAndDrain(func() error { + // A leaked synthesized-grant layer session (possible only if a + // panic unwound past the expansion driver's Abort) has a + // background worker ingesting through e.db; drain it before + // tearing the DB down. This runs AFTER the gate's drain so no + // in-flight Add/Finish (which run under withWrite) can be + // touching the session concurrently — Abort itself enters no + // gate, only synthLayerMu for the pointer handoff, and is a + // no-op when no session is open. + _ = e.AbortSynthesizedGrantLayer(context.Background()) + // Hold writeMu for the teardown: the gate only covers admitted + // operations, while CheckpointTo takes writeMu directly (no gate + // participation for its cut). A CheckpointTo that passed its + // closing check but hasn't locked yet must find either the mutex + // held or db nil'd under the lock — never a db torn down + // mid-checkpoint. + e.writeMu.Lock() + defer e.writeMu.Unlock() + // Invariant: flush before close on any write path. This drives the + // memtable out to an SST so a Close is never the step that leaves + // un-materialized writes behind — independent of whether EndSync or + // CheckpointTo (which flush for their own reasons) ran first. Skipped + // in read-only mode, where Flush is illegal and there is nothing to + // harden. A no-op when the memtable is already empty. + var err error + if !e.opts.readOnly { + if ferr := e.db.FlushMemtables(); ferr != nil { + err = fmt.Errorf("flush during close: %w", ferr) + } } - } - err = errors.Join(err, e.db.Close()) - e.db = nil - // Release the cache if we minted it (no shared cache). - if e.opts.sharedCache == nil && e.pebbleOpts != nil && e.pebbleOpts.Cache != nil { - e.pebbleOpts.Cache.Unref() - } - return err + err = errors.Join(err, e.db.Close()) + e.db = nil + // Release the cache if we minted it (no shared cache). + if e.opts.sharedCache == nil && e.pebbleOpts != nil && e.pebbleOpts.Cache != nil { + e.pebbleOpts.Cache.Unref() + } + return err + }) } // SetCurrentSync sets the engine's tracked current sync_id from a @@ -373,6 +402,7 @@ func (e *Engine) bindCurrentSync(syncID string) error { } e.currentSyncMu.Lock() e.currentSync = idBytes + e.currentSyncGen++ e.freshSync = false e.freshGrantsEmpty = false e.freshResourcesEmpty = false @@ -451,6 +481,7 @@ func (e *Engine) MarkFreshSync(syncID string) error { } e.currentSyncMu.Lock() e.currentSync = idBytes + e.currentSyncGen++ e.freshSync = true e.freshGrantsEmpty = true e.freshResourcesEmpty = true @@ -468,6 +499,7 @@ func (e *Engine) MarkFreshSync(syncID string) error { func (e *Engine) clearCurrentSync() { e.currentSyncMu.Lock() e.currentSync = nil + e.currentSyncGen++ e.freshSync = false e.freshGrantsEmpty = false e.freshResourcesEmpty = false @@ -520,8 +552,9 @@ func (e *Engine) takeFreshResourcesEmpty() bool { // before the caller returns. Called by Adapter.EndSync. // // Uses withWrite (not a bare writeMu) so the flush participates in the -// closing check and writeWG: Close tears e.db down after writeWG.Wait, -// and a bare-mutex EndFreshSync racing Close would flush a nil db. +// closing check and the close drain: Close tears e.db down only after +// admitted writes exit, and a bare-mutex EndFreshSync racing Close would +// flush a nil db. func (e *Engine) EndFreshSync(ctx context.Context) error { // AllowSealed: this is the last step of EndSync's sealed finalize // window (see Adapter.EndSync). @@ -572,6 +605,14 @@ func (e *Engine) CurrentSyncID() string { return codec.DecodeSyncID(e.currentSync) } +// currentSyncBinding returns the bound sync's id together with the +// generation of that binding, as one snapshot. +func (e *Engine) currentSyncBinding() (string, uint64) { + e.currentSyncMu.RLock() + defer e.currentSyncMu.RUnlock() + return codec.DecodeSyncID(e.currentSync), e.currentSyncGen +} + // requireCurrentSync returns ErrNoCurrentSync unless a sync is bound // (StartNewSync/SetCurrentSync, cleared by EndSync). Record writes // gate on this so data never lands without a sync-run record — the @@ -603,11 +644,13 @@ func (e *Engine) checkWritable() error { // checkWritableAllowSealed is checkWritable without the sealed check, for // the few write paths that legitimately run on a finished sync (sync-run // metadata updates and the pre-StartNewSync wipe). +// +// No e.db nil check here on purpose: reading the field outside the gate +// is exactly the unsynchronized access the gate exists to remove (a data +// race under -race), and it can never catch anything the closing check +// misses — the teardown nils the field only after the flip this reads. func (e *Engine) checkWritableAllowSealed() error { - if e.closing.Load() { - return ErrEngineClosing - } - if e.db == nil { + if e.admit.isClosing() { return ErrEngineClosing } if e.opts.readOnly { @@ -646,18 +689,43 @@ func (e *Engine) withWriteAllowSealed(fn func() error) error { if err := e.checkWritableAllowSealed(); err != nil { return err } - e.writeWG.Add(1) - defer e.writeWG.Done() - // Re-check after Add because closing could have flipped between - // our first check and our Add. - if e.closing.Load() { - return ErrEngineClosing + // checkWritableAllowSealed above reads the closing flag, but a pass + // there is only advisory — the gate's admission is the check that + // counts. + if err := e.admit.enterWrite(); err != nil { + return err } - e.writeMu.Lock() - defer e.writeMu.Unlock() + defer e.admit.exitWrite() + e.lockWriteBarrier() + defer e.unlockWriteBarrier() return fn() } +// pinRead pins the engine's handle open for one read and returns it. +// +// Reads take no barrier — concurrency between them and with writes is +// the point — so this is not a lock. What it buys is the two things +// Close needs: the handle cannot be torn down while a read is using it +// (Close drains admitted reads), and the read's view of e.db is ordered +// against Close's teardown by the gate instead of being an +// unsynchronized field access. +// +// Callers must defer the returned release, and must read through the +// returned handle rather than e.db — re-reading the field inside the +// body reintroduces exactly the unordered access this removes. +// TestScanReadsArePinned holds the read surface to both. +func (e *Engine) pinRead() (*rawdb.DB, func(), error) { + if err := e.admit.enterRead(); err != nil { + return nil, nil, err + } + // Safe unsynchronized, and non-nil by construction: admission + // precedes Close's flip, so Close is still waiting on this pin and + // has not reached the teardown that nils the field. (A nil check + // here would be dead code guarding an ordering the gate already + // provides.) + return e.db, e.admit.exitRead, nil +} + func (e *Engine) Save(ctx context.Context, dest string) error { return errors.New("pebble engine: Save requires the dotc1z.Save shim (envelope write); use CheckpointTo for direct directory access") } @@ -818,10 +886,11 @@ func (e *Engine) removeStagingDir(dir string) { // (see ingestSynthLayerSegment), and a flushable ingest landing mid-window // would be a WAL-only record the truncate discards. func (e *Engine) CheckpointTo(ctx context.Context, destDir string) error { - // Wait for all in-flight writes to complete. - e.writeWG.Wait() + // Wait for all in-flight writes to complete (panics rather than + // self-deadlocking if called from inside one). + e.admit.drainWrites() - if e.closing.Load() { + if e.admit.isClosing() { return ErrEngineClosing } e.writeMu.Lock() @@ -830,7 +899,7 @@ func (e *Engine) CheckpointTo(ctx context.Context, destDir string) error { defer e.checkpointMu.Unlock() // Re-check under the lock: Close (which also takes writeMu for its // teardown) may have won the race and nil'd e.db. - if e.closing.Load() || e.db == nil { + if e.admit.isClosing() || e.db == nil { return ErrEngineClosing } diff --git a/pkg/dotc1z/engine/pebble/entitlements.go b/pkg/dotc1z/engine/pebble/entitlements.go index e9e728e26..b5ab5e132 100644 --- a/pkg/dotc1z/engine/pebble/entitlements.go +++ b/pkg/dotc1z/engine/pebble/entitlements.go @@ -91,11 +91,16 @@ func (e *Engine) PutEntitlementRecords(ctx context.Context, records ...*v3.Entit // GetEntitlementRecord fetches an entitlement by its raw public id via the // bare-id lookup (exact string-match, exactly-one rule — see lookup.go). func (e *Engine) GetEntitlementRecord(ctx context.Context, externalID string) (*v3.EntitlementRecord, error) { - id, err := e.resolveEntitlementIdentityByExternalID(ctx, externalID) + db, release, err := e.pinRead() if err != nil { return nil, err } - val, closer, err := e.db.Get(encodeEntitlementIdentityKey(id)) + defer release() + id, err := e.resolveEntitlementIdentityByExternalID(ctx, db, externalID) + if err != nil { + return nil, err + } + val, closer, err := db.Get(encodeEntitlementIdentityKey(id)) if err != nil { return nil, err } @@ -112,7 +117,8 @@ func (e *Engine) GetEntitlementRecord(ctx context.Context, externalID string) (* // delete). func (e *Engine) DeleteEntitlementRecord(ctx context.Context, externalID string) error { return e.withWrite(func() error { - id, err := e.resolveEntitlementIdentityByExternalID(ctx, externalID) + // e.db is the admitted write's stable handle here. + id, err := e.resolveEntitlementIdentityByExternalID(ctx, e.db, externalID) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return nil @@ -160,8 +166,13 @@ func (e *Engine) DeleteEntitlementRecordByIdentity( } func (e *Engine) IterateEntitlements(ctx context.Context, yield func(*v3.EntitlementRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() prefix := encodeEntitlementPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -170,6 +181,9 @@ func (e *Engine) IterateEntitlements(ctx context.Context, yield func(*v3.Entitle } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } r := &v3.EntitlementRecord{} if err := unmarshalRecord(iter.Value(), r); err != nil { return fmt.Errorf("iterate entitlements: %w", err) @@ -182,8 +196,13 @@ func (e *Engine) IterateEntitlements(ctx context.Context, yield func(*v3.Entitle } func (e *Engine) IterateEntitlementsByResource(ctx context.Context, resourceTypeID, resourceID string, yield func(*v3.EntitlementRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() indexPrefix := encodeEntitlementPrimaryResourcePrefix(resourceTypeID, resourceID) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: indexPrefix, UpperBound: upperBoundOf(indexPrefix), }) @@ -192,6 +211,9 @@ func (e *Engine) IterateEntitlementsByResource(ctx context.Context, resourceType } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } r := &v3.EntitlementRecord{} if err := unmarshalRecord(iter.Value(), r); err != nil { return err diff --git a/pkg/dotc1z/engine/pebble/grant_digest.go b/pkg/dotc1z/engine/pebble/grant_digest.go index b173e5de1..3e3b68b52 100644 --- a/pkg/dotc1z/engine/pebble/grant_digest.go +++ b/pkg/dotc1z/engine/pebble/grant_digest.go @@ -350,7 +350,12 @@ func grantPrimaryKeyFromHashIndexKey(dst, idxKey []byte) ([]byte, bool) { // "zero grants" for an entitlement that may have millions — the // false-clean trap dirtyPartitionBuckets' doc comment describes. func (e *Engine) GetEntitlementDigestRoot(ctx context.Context, id entitlementIdentity) (DigestRoot, bool, error) { - return e.getPartitionDigestRoot(grantDigestSpec, digestPartitionForEntitlement(id)) + db, release, err := e.pinRead() + if err != nil { + return DigestRoot{}, false, err + } + defer release() + return e.getPartitionDigestRoot(db, grantDigestSpec, digestPartitionForEntitlement(id)) } // GetGrantDigestGlobalRoot returns the whole-file grant digest root — @@ -369,7 +374,12 @@ func (e *Engine) GetGrantDigestGlobalRoot(ctx context.Context) (DigestRoot, bool // hash index that was never ingested. return DigestRoot{}, false, nil } - val, closer, err := e.db.Get(rawdb.GlobalGrantDigestNodeKey()) + db, release, err := e.pinRead() + if err != nil { + return DigestRoot{}, false, err + } + defer release() + val, closer, err := db.Get(rawdb.GlobalGrantDigestNodeKey()) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return DigestRoot{}, false, nil @@ -397,7 +407,12 @@ func (e *Engine) GetGrantDigestGlobalRoot(ctx context.Context) (DigestRoot, bool // Never use it as a fallback for a missing root; see // GetEntitlementDigestRoot and computeBucketDigest's precondition. func (e *Engine) ComputeEntitlementBucketDigest(ctx context.Context, id entitlementIdentity, bucket DigestBucket) ([]byte, int64, error) { - return e.computeBucketDigest(ctx, grantDigestSpec, digestPartitionForEntitlement(id), bucket) + db, release, err := e.pinRead() + if err != nil { + return nil, 0, err + } + defer release() + return e.computeBucketDigest(ctx, db, grantDigestSpec, digestPartitionForEntitlement(id), bucket) } // DirtyEntitlementBuckets compares this engine's entitlement against @@ -416,8 +431,13 @@ func (e *Engine) DirtyEntitlementBuckets(ctx context.Context, other *Engine, id // (no decode); the point Get per entry is the cost of MATERIALIZING a // changed grant, not of finding it. Orphan index entries are skipped. func (e *Engine) IterateGrantsByEntitlementBucket(ctx context.Context, id entitlementIdentity, bucket DigestBucket, yield func(*v3.GrantRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() lower, upper := grantDigestSpec.bucketBounds(digestPartitionForEntitlement(id), bucket) - iter, err := e.db.NewIter(&pebble.IterOptions{LowerBound: lower, UpperBound: upper}) + iter, err := db.NewIter(&pebble.IterOptions{LowerBound: lower, UpperBound: upper}) if err != nil { return err } @@ -432,7 +452,7 @@ func (e *Engine) IterateGrantsByEntitlementBucket(ctx context.Context, id entitl if !ok { continue } - val, closer, getErr := e.db.Get(priKey) + val, closer, getErr := db.Get(priKey) if getErr != nil { if errors.Is(getErr, pebble.ErrNotFound) { continue diff --git a/pkg/dotc1z/engine/pebble/grant_digest_repair.go b/pkg/dotc1z/engine/pebble/grant_digest_repair.go index ef93fb070..c313ac929 100644 --- a/pkg/dotc1z/engine/pebble/grant_digest_repair.go +++ b/pkg/dotc1z/engine/pebble/grant_digest_repair.go @@ -103,10 +103,15 @@ func isGrantDigestRootKey(key []byte) bool { // an O(file) rebuild. No-op when partitions is empty or no digest has // ever been built for this file. func (e *Engine) InvalidateGrantDigestPartitions(ctx context.Context, partitions []string) error { - if len(partitions) == 0 || !e.db.GrantDigestsPresent() { + if len(partitions) == 0 { return nil } return e.withWrite(func() error { + // Checked inside the admitted write: reading the handle before + // admission was the bare access the gate exists to remove. + if !e.db.GrantDigestsPresent() { + return nil + } batch := e.db.NewDigestBatch() defer batch.Close() for _, partition := range partitions { @@ -201,10 +206,21 @@ func (e *Engine) RepairMissingGrantDigests(ctx context.Context) error { return fmt.Errorf("RepairMissingGrantDigests: drop digest state left by an interrupted build: %w", err) } } - if !e.db.GrantDigestsPresent() { + present, err := func() (bool, error) { + db, release, err := e.pinRead() + if err != nil { + return false, err + } + defer release() + return db.GrantDigestsPresent(), nil + }() + if err != nil { + return err + } + if !present { return e.BuildGrantDigests(ctx) } - err := e.repairMissingGrantDigestsAttempt(ctx) + err = e.repairMissingGrantDigestsAttempt(ctx) if err == nil { return nil } diff --git a/pkg/dotc1z/engine/pebble/grants.go b/pkg/dotc1z/engine/pebble/grants.go index a603dfcab..a30d3da1a 100644 --- a/pkg/dotc1z/engine/pebble/grants.go +++ b/pkg/dotc1z/engine/pebble/grants.go @@ -894,12 +894,17 @@ func (e *Engine) UnsafePutUniqueGrantRecords(ctx context.Context, records ...*v3 // GetGrantRecord fetches a grant record by its raw public id via the // bare-id lookup (candidate-split probing, exactly-one rule — lookup.go). func (e *Engine) GetGrantRecord(ctx context.Context, externalID string) (*v3.GrantRecord, error) { - id, err := e.resolveGrantIdentityByExternalID(ctx, externalID) + db, release, err := e.pinRead() + if err != nil { + return nil, err + } + defer release() + id, err := e.resolveGrantIdentityByExternalID(ctx, db, externalID) if err != nil { return nil, err } key := encodeGrantIdentityKey(id) - val, closer, err := e.db.Get(key) + val, closer, err := db.Get(key) if err != nil { return nil, err } @@ -916,7 +921,9 @@ func (e *Engine) GetGrantRecord(ctx context.Context, externalID string) (*v3.Gra // lossy string must never guess a delete). func (e *Engine) DeleteGrantRecord(ctx context.Context, externalID string) error { return e.withWrite(func() error { - id, err := e.resolveGrantIdentityByExternalID(ctx, externalID) + // e.db is the admitted write's stable handle here (withWrite + // holds gate admission, so teardown cannot run). + id, err := e.resolveGrantIdentityByExternalID(ctx, e.db, externalID) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return nil @@ -1013,8 +1020,13 @@ func (e *Engine) clearDeferredIdxPending() error { // IterateGrants iterates all grants in primary-key order. yield returns // false to stop iteration. func (e *Engine) IterateGrants(ctx context.Context, yield func(*v3.GrantRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() prefix := encodeGrantPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -1023,6 +1035,9 @@ func (e *Engine) IterateGrants(ctx context.Context, yield func(*v3.GrantRecord) } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } r := &v3.GrantRecord{} if err := unmarshalRecord(iter.Value(), r); err != nil { return fmt.Errorf("iterate grants: %w", err) @@ -1039,7 +1054,15 @@ func (e *Engine) IterateGrants(ctx context.Context, yield func(*v3.GrantRecord) // The id resolves through the bare-id lookup; an id matching no entitlement // iterates nothing. yield returns false to stop. func (e *Engine) IterateGrantsByEntitlement(ctx context.Context, entitlementID string, yield func(*v3.GrantRecord) bool) error { - entID, err := e.resolveGrantScanEntitlementIdentity(ctx, entitlementID) + // Pinned before the identity resolve: that lookup reads the handle + // too, so leaving it outside would protect the scan and not the + // query that feeds it. + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() + entID, err := e.resolveGrantScanEntitlementIdentity(ctx, db, entitlementID) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return nil @@ -1047,7 +1070,7 @@ func (e *Engine) IterateGrantsByEntitlement(ctx context.Context, entitlementID s return err } indexPrefix := encodeGrantPrimaryEntitlementPrefix(entID) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: indexPrefix, UpperBound: upperBoundOf(indexPrefix), }) @@ -1056,6 +1079,9 @@ func (e *Engine) IterateGrantsByEntitlement(ctx context.Context, entitlementID s } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } r := &v3.GrantRecord{} if err := unmarshalRecord(iter.Value(), r); err != nil { return fmt.Errorf("iterate by entitlement: %w", err) @@ -1069,8 +1095,13 @@ func (e *Engine) IterateGrantsByEntitlement(ctx context.Context, entitlementID s // IterateGrantsByPrincipal iterates the by_principal index. func (e *Engine) IterateGrantsByPrincipal(ctx context.Context, principalRT, principalID string, yield func(*v3.GrantRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() indexPrefix := encodeGrantByPrincipalPrefix(principalRT, principalID) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: indexPrefix, UpperBound: upperBoundOf(indexPrefix), }) @@ -1079,11 +1110,14 @@ func (e *Engine) IterateGrantsByPrincipal(ctx context.Context, principalRT, prin } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } components, ok := decodeTupleComponents(iter.Key(), indexPrefix, 4) if !ok { continue } - r, err := getGrantByIdentity(ctx, e.db, grantIdentity{ + r, err := getGrantByIdentity(ctx, db, grantIdentity{ entitlement: entitlementIdentity{ resourceTypeID: components[0], resourceID: components[1], @@ -1110,8 +1144,13 @@ func (e *Engine) IterateGrantsByPrincipal(ctx context.Context, principalRT, prin // to a principal resource type. Yields each grant whose principal carries the // given resource_type. Stops when yield returns false. func (e *Engine) IterateGrantsByPrincipalResourceType(ctx context.Context, principalRT string, yield func(*v3.GrantRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() indexPrefix := encodeGrantByPrincipalResourceTypeIdentityPrefix(principalRT) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: indexPrefix, UpperBound: upperBoundOf(indexPrefix), }) @@ -1120,11 +1159,14 @@ func (e *Engine) IterateGrantsByPrincipalResourceType(ctx context.Context, princ } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } components, ok := decodeTupleComponents(iter.Key(), indexPrefix, 5) if !ok { continue } - r, err := getGrantByIdentity(ctx, e.db, grantIdentity{ + r, err := getGrantByIdentity(ctx, db, grantIdentity{ entitlement: entitlementIdentity{ resourceTypeID: components[1], resourceID: components[2], @@ -1155,8 +1197,13 @@ func (e *Engine) IterateGrantsByPrincipalResourceType(ctx context.Context, princ // `WHERE needs_expansion = 1`. Backs PendingExpansionPage on the // grant store. func (e *Engine) IterateGrantsByNeedsExpansion(ctx context.Context, yield func(*v3.GrantRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() indexPrefix := encodeGrantByNeedsExpansionPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: indexPrefix, UpperBound: upperBoundOf(indexPrefix), }) @@ -1165,11 +1212,14 @@ func (e *Engine) IterateGrantsByNeedsExpansion(ctx context.Context, yield func(* } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } components, ok := decodeTupleComponents(iter.Key(), indexPrefix, 6) if !ok { continue } - r, err := getGrantByIdentity(ctx, e.db, grantIdentity{ + r, err := getGrantByIdentity(ctx, db, grantIdentity{ entitlement: entitlementIdentity{ resourceTypeID: components[0], resourceID: components[1], diff --git a/pkg/dotc1z/engine/pebble/handle_access_meta_test.go b/pkg/dotc1z/engine/pebble/handle_access_meta_test.go new file mode 100644 index 000000000..d31bbddf8 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/handle_access_meta_test.go @@ -0,0 +1,253 @@ +package pebble + +// Meta-tests keyed on the handle FIELD ACCESS itself, not on method +// naming. TestScanReadsArePinned holds the named read families +// (Paginate/Iterate/ForEach) to the pinRead pattern; these two tests +// close the hole it leaves: a read added under any other name used to +// escape enforcement entirely, and that is exactly how the unpinned +// point-read surface (GetGrantRecord and friends) accumulated. +// +// The contract they pin: +// +// - e.db may be touched only from admitted contexts. Syntactically +// that means: inside a function literal passed to withWrite / +// withWriteAllowSealed (the write path's admission), or inside one +// of the enumerated functions below — each of which is admitted by +// construction (gate/lifecycle machinery, Open-time code before +// any reader exists, helpers documented as running only under an +// admitted write, or the merge surface's documented exclusion). +// Everything else must pin (pinRead) and use the handle pinRead +// returns, or take an admitted handle as a parameter. +// +// - every pinRead's release must be deferred in the same function, +// so no early return can leak an admission and wedge Close. + +import ( + "fmt" + "go/ast" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// admittedDBAccessors enumerates every function permitted to touch e.db +// outside a withWrite/withWriteAllowSealed literal, and why. Additions +// here need the same justification as an entry in a lock-ordering +// inventory: say which admission covers the access. +var admittedDBAccessors = map[string]string{ + // Gate and lifecycle machinery: these ARE the admission. + "pinRead": "returns the handle under a read admission it just took", + "Close": "teardown closure runs inside closeAndDrain, after both drains", + "CheckpointTo": "flush/checkpoint runs under the write barrier with writes drained", + + // Direct gate participants: they call admit.enterWrite themselves + // (pebble compaction/flush are concurrency-safe with foreground + // writes, so they skip writeMu — see their doc comments). + "CompactAllRanges": "holds enterWrite for the duration", + "Flush": "holds enterWrite for the duration", + + // Open-time, before the engine is shared: no reader or Close can + // exist yet. + "Open": "constructor, pre-share", + "verifyOrStampKeyspaceVersion": "Open-time, pre-share", + "stampKeyspaceVersion": "Open-time, pre-share", + "isKeyspaceEmpty": "Open-time, pre-share", + "isDataKeyspaceEmpty": "Open-time, pre-share", + "readIDIndexFormat": "Open-time, pre-share", + "writeIDIndexFormat": "Open-time migration, pre-share", + "readAppliedIndexVersion": "Open-time migration, pre-share", + "writeAppliedIndexVersion": "Open-time migration, pre-share", + "migrateIDIndexFormatToStructuredV1": "Open-time migration, pre-share", + "emitStructuredEntitlementMigration": "Open-time migration, pre-share", + "emitStructuredGrantMigration": "Open-time migration, pre-share", + "replaceRangeWithSST": "Open-time migration, pre-share", + + // Helpers called only from admitted writes (their callers hold + // write admission; several are named *Locked for exactly this). + "hasSyncRun": "called only from startNewSync, an admitted write", + "endSyncFinalize": "called only from EndSync, an admitted write", + "deleteGrantByIdentityLocked": "caller holds withWrite", + "ingestSynthLayerSegment": "called only from the synth-layer flush inside withWrite", + "buildDeferredGrantIndexesLocked": "caller holds withWriteAllowSealed", + "markGrantDigestBuildPending": "called only under the digest build's write admission", + "clearGrantDigestBuildPending": "called only under write admission (build, cleanup, drop)", + "newGrantDigestFold": "constructed only under the digest build's write admission", + "closePartition": "grantDigestFold internals; build holds write admission", + "buildGrantDigestsFromSpill": "callers hold withWriteAllowSealed", + "buildGrantDigestsStandaloneLocked": "caller holds withWriteAllowSealed", + "writeMissingEntitlementDigestRoots": "called only from the digest build, admitted", + "dropAllGrantDigestStateLocked": "callers hold withWriteAllowSealed (or Open, pre-share)", + "findMissingGrantDigestPartitionsLocked": "repair holds write admission", + "repairOneGrantDigestPartitionLocked": "repair holds write admission", + "recomputeGrantDigestGlobalRootLocked": "repair holds write admission", + "grantDigestRootPresent": "called only from the locked repair scan", + "foldPartitionNodes": "called only under digest build/repair write admission", +} + +// admittedDBAccessorFiles are files whose every function is admitted +// wholesale, with the same justification discipline. +var admittedDBAccessorFiles = map[string]string{ + "merge_surface.go": "documented admission-gate exclusion; see file header", + "merge_accessor.go": "same compactor ordering fence as merge_surface.go", +} + +// insideWithWriteLiteral reports whether the node path (outermost +// first) passes through a function literal that is an argument to +// withWrite or withWriteAllowSealed. +func insideWithWriteLiteral(path []ast.Node) bool { + for i, n := range path { + if _, ok := n.(*ast.FuncLit); !ok || i == 0 { + continue + } + call, ok := path[i-1].(*ast.CallExpr) + if !ok { + continue + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + continue + } + if sel.Sel.Name == "withWrite" || sel.Sel.Name == "withWriteAllowSealed" { + return true + } + } + return false +} + +// walkWithPath drives visit with the ancestry (outermost first) of +// every node. +func walkWithPath(root ast.Node, visit func(path []ast.Node, n ast.Node)) { + var path []ast.Node + ast.Inspect(root, func(n ast.Node) bool { + if n == nil { + path = path[:len(path)-1] + return true + } + visit(path, n) + path = append(path, n) + return true + }) +} + +func enclosingFuncDecl(path []ast.Node) *ast.FuncDecl { + for i := len(path) - 1; i >= 0; i-- { + if fd, ok := path[i].(*ast.FuncDecl); ok { + return fd + } + } + return nil +} + +func TestBareHandleAccessIsGateCovered(t *testing.T) { + fset, files := parseProductionDir(t, ".") + + var violations []string + for name, f := range files { + base := name[strings.LastIndexByte(name, '/')+1:] + if _, ok := admittedDBAccessorFiles[base]; ok { + continue + } + walkWithPath(f, func(path []ast.Node, n ast.Node) { + sel, ok := n.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "db" { + return + } + // Engine handle accesses only: `e.db` off the conventional + // receiver, or a chain ending in an `e` field (f.e.db, + // b.e.db). Other types' db fields (the compaction + // scheduler's pebble.DBForCompaction) are not this gate's + // concern. + switch x := sel.X.(type) { + case *ast.Ident: + if x.Name != "e" { + return + } + case *ast.SelectorExpr: + if x.Sel.Name != "e" { + return + } + default: + return + } + if insideWithWriteLiteral(append(append([]ast.Node{}, path...), n)) { + return + } + fd := enclosingFuncDecl(path) + if fd == nil { + return + } + if _, ok := admittedDBAccessors[fd.Name.Name]; ok { + return + } + violations = append(violations, + fmt.Sprintf("%s: %s", fset.Position(sel.Pos()), fd.Name.Name)) + }) + } + sort.Strings(violations) + require.Empty(t, violations, + "bare e.db access outside every admitted context. The handle may only be touched under gate admission: "+ + "pin the read (pinRead) and use the handle it returns, plumb an admitted handle parameter, do the write "+ + "inside withWrite/withWriteAllowSealed, or — if the function really is admitted by construction — add it "+ + "to admittedDBAccessors with a justification.\n%s", strings.Join(violations, "\n")) +} + +func TestPinnedReadsDeferTheirRelease(t *testing.T) { + fset, files := parseProductionDir(t, ".") + + checked := 0 + for _, f := range files { + walkWithPath(f, func(path []ast.Node, n ast.Node) { + assign, ok := n.(*ast.AssignStmt) + if !ok || len(assign.Rhs) != 1 || len(assign.Lhs) != 3 { + return + } + call, ok := assign.Rhs[0].(*ast.CallExpr) + if !ok { + return + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "pinRead" { + return + } + checked++ + relIdent, ok := assign.Lhs[1].(*ast.Ident) + require.True(t, ok && relIdent.Name != "_", + "%s: pinRead's release discarded — the admission can never be returned and Close will hang", + fset.Position(assign.Pos())) + + // The innermost enclosing function body owns the defer. + var body *ast.BlockStmt + for i := len(path) - 1; i >= 0; i-- { + switch fn := path[i].(type) { + case *ast.FuncLit: + body = fn.Body + case *ast.FuncDecl: + body = fn.Body + } + if body != nil { + break + } + } + require.NotNil(t, body, "%s: pinRead outside any function?", fset.Position(assign.Pos())) + + deferred := false + ast.Inspect(body, func(m ast.Node) bool { + d, ok := m.(*ast.DeferStmt) + if !ok { + return true + } + if id, ok := d.Call.Fun.(*ast.Ident); ok && id.Name == relIdent.Name { + deferred = true + } + return true + }) + require.True(t, deferred, + "%s: pinRead's release (%q) is not deferred in the same function. An early return between the pin "+ + "and an explicit release leaks the admission, and Close waits on it forever", + fset.Position(assign.Pos()), relIdent.Name) + }) + } + require.Positive(t, checked, "no pinRead call sites found: this check has drifted off the surface it holds") +} diff --git a/pkg/dotc1z/engine/pebble/ingest_facts.go b/pkg/dotc1z/engine/pebble/ingest_facts.go index 6cc7d85c5..95d40d056 100644 --- a/pkg/dotc1z/engine/pebble/ingest_facts.go +++ b/pkg/dotc1z/engine/pebble/ingest_facts.go @@ -18,6 +18,7 @@ import ( v3 "github.com/conductorone/baton-sdk/pb/c1/storage/v3" "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble/codec" + "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble/internal/rawdb" ) // Annotation type names the invariant probes match against (the tail @@ -72,11 +73,13 @@ func grantValueCarriesInsertFact(val []byte) (bool, error) { // distinct resource — O(distinct) seeks, never O(grants). Backs the // syncer's grant→resource referential invariant (I3). func (e *Engine) ForEachDistinctGrantEntitlementResource(ctx context.Context, visit func(resourceTypeID, resourceID string) error) error { - if e.db == nil { - return ErrEngineClosing + db, release, err := e.pinRead() + if err != nil { + return err } + defer release() prefix := encodeGrantPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -119,11 +122,13 @@ func (e *Engine) ForEachDistinctGrantEntitlementResource(ctx context.Context, vi // scan: one seek per distinct resource, never O(entitlements). Backs // the syncer's entitlement→resource referential invariant (I7). func (e *Engine) ForEachDistinctEntitlementResource(ctx context.Context, visit func(resourceTypeID, resourceID string) error) error { - if e.db == nil { - return ErrEngineClosing + db, release, err := e.pinRead() + if err != nil { + return err } + defer release() prefix := encodeEntitlementPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -169,11 +174,13 @@ func (e *Engine) ForEachDistinctEntitlementResource(ctx context.Context, visit f // (see identity.go). Backs the syncer's grant→entitlement referential // invariant (I8). func (e *Engine) ForEachDanglingGrantEntitlement(ctx context.Context, visit func(entitlementID, resourceTypeID, resourceID string) error) error { - if e.db == nil { - return ErrEngineClosing + db, release, err := e.pinRead() + if err != nil { + return err } + defer release() prefix := encodeGrantPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -208,7 +215,7 @@ func (e *Engine) ForEachDanglingGrantEntitlement(ctx context.Context, visit func stripped: comps[2] == idFlagStripped, tail: comps[3], } - exists, err := e.hasEntitlementIdentity(id) + exists, err := hasEntitlementIdentity(db, id) if err != nil { return err } @@ -223,8 +230,8 @@ func (e *Engine) ForEachDanglingGrantEntitlement(ctx context.Context, visit func return iter.Error() } -func (e *Engine) hasEntitlementIdentity(id entitlementIdentity) (bool, error) { - _, closer, err := e.db.Get(encodeEntitlementIdentityKey(id)) +func hasEntitlementIdentity(db *rawdb.DB, id entitlementIdentity) (bool, error) { + _, closer, err := db.Get(encodeEntitlementIdentityKey(id)) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return false, nil @@ -245,12 +252,14 @@ func (e *Engine) hasEntitlementIdentity(id entitlementIdentity) (bool, error) { // reference it). Reads row values, so it is reserved for DANGLING // referential probes — rare to zero on healthy syncs. func (e *Engine) GrantsForEntitlementAllCarryInsertFact(ctx context.Context, entitlementID, entResourceTypeID, entResourceID string) (bool, error) { - if e.db == nil { - return false, ErrEngineClosing + db, release, err := e.pinRead() + if err != nil { + return false, err } + defer release() entID := entitlementIdentityFromParts(entResourceTypeID, entResourceID, entitlementID) prefix := encodeGrantPrimaryEntitlementPrefix(entID) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -278,11 +287,13 @@ func (e *Engine) GrantsForEntitlementAllCarryInsertFact(ctx context.Context, ent // Reads row values, so it is reserved for DANGLING referential probes — // rare to zero on healthy syncs — never the bulk path. func (e *Engine) GrantsForEntResourceCarryInsertFact(ctx context.Context, resourceTypeID, resourceID string) (bool, error) { - if e.db == nil { - return false, ErrEngineClosing + db, release, err := e.pinRead() + if err != nil { + return false, err } + defer release() prefix := encodeGrantPrimaryEntitlementResourcePrefix(resourceTypeID, resourceID) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -308,10 +319,19 @@ func (e *Engine) GrantsForEntResourceCarryInsertFact(ctx context.Context, resour // HasResourceRecord reports whether a resource row exists — the probe // side of the referential invariants. func (e *Engine) HasResourceRecord(ctx context.Context, resourceTypeID, resourceID string) (bool, error) { - if e.db == nil { - return false, ErrEngineClosing + db, release, err := e.pinRead() + if err != nil { + return false, err } - _, closer, err := e.db.Get(encodeResourceKey(resourceTypeID, resourceID)) + defer release() + return hasResourceRecordOn(db, resourceTypeID, resourceID) +} + +// hasResourceRecordOn is HasResourceRecord against a handle the caller +// already holds admitted — used inside pinned scans so the probe doesn't +// re-pin (and can't be refused mid-scan by a concurrent Close). +func hasResourceRecordOn(db *rawdb.DB, resourceTypeID, resourceID string) (bool, error) { + _, closer, err := db.Get(encodeResourceKey(resourceTypeID, resourceID)) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return false, nil diff --git a/pkg/dotc1z/engine/pebble/ingest_repair.go b/pkg/dotc1z/engine/pebble/ingest_repair.go index 9225caa70..39f6926ec 100644 --- a/pkg/dotc1z/engine/pebble/ingest_repair.go +++ b/pkg/dotc1z/engine/pebble/ingest_repair.go @@ -19,6 +19,7 @@ import ( v3 "github.com/conductorone/baton-sdk/pb/c1/storage/v3" "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble/codec" + "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble/internal/rawdb" ) // EnsureGrantIndexes runs the deferred grant-index build NOW if one is @@ -38,10 +39,22 @@ import ( // post-collection seam guarantees quiescence (parallelSync has // drained); any new caller must guarantee the same or use EndSync. func (e *Engine) EnsureGrantIndexes(ctx context.Context) error { - if e.db == nil { - return ErrEngineClosing + // The pending probe reads the handle, so it needs gate admission like + // any other read; the build and clear below take their own write + // admission. (The old bare e.db nil check was the unsynchronized + // access the gate replaces.) + pending, err := func() (bool, error) { + db, release, err := e.pinRead() + if err != nil { + return false, err + } + defer release() + return db.DeferredIdxPending(), nil + }() + if err != nil { + return err } - if !e.db.DeferredIdxPending() { + if !pending { return nil } if err := e.BuildDeferredGrantIndexes(ctx); err != nil { @@ -71,12 +84,14 @@ func (e *Engine) EnsureGrantIndexes(ctx context.Context) error { // the orphan index keys are deleted — instead of being vacuously // classified as match-annotated-only. func (e *Engine) ForEachDanglingGrantPrincipal(ctx context.Context, visit func(principalRT, principalID string, matchAnnotatedOnly bool, carrierGrants int64) error) error { - if e.db == nil { - return ErrEngineClosing + db, release, err := e.pinRead() + if err != nil { + return err } + defer release() prefix := []byte{versionV3, typeIndex, idxGrantByPrincipal} prefix = codec.AppendTupleSeparator(prefix) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -123,12 +138,15 @@ func (e *Engine) ForEachDanglingGrantPrincipal(ctx context.Context, visit func(p return fmt.Errorf("dangling grant-principal scan: malformed index key %x", key) } rt, rid := string(rtBytes), string(ridBytes) - exists, err := e.HasResourceRecord(ctx, rt, rid) + // The pinned handle threads through the probes: re-pinning inside + // an already-admitted scan would let a concurrent Close refuse the + // inner probe mid-operation. + exists, err := hasResourceRecordOn(db, rt, rid) if err != nil { return err } if !exists { - matchOnly, carrierGrants, err := e.grantsForPrincipalAllMatchAnnotated(ctx, rt, rid) + matchOnly, carrierGrants, err := grantsForPrincipalAllMatchAnnotated(ctx, db, rt, rid) if err != nil { return err } @@ -191,7 +209,8 @@ func (e *Engine) invariantWriteOpts() *pebble.WriteOptions { func (e *Engine) healOrphanPrincipalIndexEntries(ctx context.Context, principalRT, principalID string) (int64, error) { var healed int64 err := e.withWrite(func() error { - ids, err := e.grantIdentitiesForPrincipal(ctx, principalRT, principalID) + // e.db is the admitted write's stable handle here. + ids, err := grantIdentitiesForPrincipal(ctx, e.db, principalRT, principalID) if err != nil { return err } @@ -240,8 +259,8 @@ func (e *Engine) healOrphanPrincipalIndexEntries(ctx context.Context, principalR // their carriers too, so the syncer's per-GRANT carrier totals don't // silently lose the mixed case — the full walk is acceptable because // this probe runs only for DANGLING principals (reads row values). -func (e *Engine) grantsForPrincipalAllMatchAnnotated(ctx context.Context, principalRT, principalID string) (bool, int64, error) { - ids, err := e.grantIdentitiesForPrincipal(ctx, principalRT, principalID) +func grantsForPrincipalAllMatchAnnotated(ctx context.Context, db *rawdb.DB, principalRT, principalID string) (bool, int64, error) { + ids, err := grantIdentitiesForPrincipal(ctx, db, principalRT, principalID) if err != nil { return false, 0, err } @@ -251,7 +270,7 @@ func (e *Engine) grantsForPrincipalAllMatchAnnotated(ctx context.Context, princi if err := ctx.Err(); err != nil { return false, 0, err } - rec, err := e.getGrantRecordByIdentity(id) + rec, err := getGrantRecordByIdentity(db, id) if err != nil { if errors.Is(err, pebble.ErrNotFound) { continue // index entry without a row; the scan tolerates it @@ -270,9 +289,9 @@ func (e *Engine) grantsForPrincipalAllMatchAnnotated(ctx context.Context, princi // grantIdentitiesForPrincipal collects the grant identities under one // principal from the by_principal index. Collected before any deletes so // callers never interleave iteration with writes. -func (e *Engine) grantIdentitiesForPrincipal(ctx context.Context, principalRT, principalID string) ([]grantIdentity, error) { +func grantIdentitiesForPrincipal(ctx context.Context, db *rawdb.DB, principalRT, principalID string) ([]grantIdentity, error) { prefix := encodeGrantByPrincipalPrefix(principalRT, principalID) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -311,8 +330,8 @@ func (e *Engine) grantIdentitiesForPrincipal(ctx context.Context, principalRT, p return ids, iter.Error() } -func (e *Engine) getGrantRecordByIdentity(id grantIdentity) (*v3.GrantRecord, error) { - val, closer, err := e.db.Get(encodeGrantIdentityKey(id)) +func getGrantRecordByIdentity(db *rawdb.DB, id grantIdentity) (*v3.GrantRecord, error) { + val, closer, err := db.Get(encodeGrantIdentityKey(id)) if err != nil { return nil, err } diff --git a/pkg/dotc1z/engine/pebble/ingest_repair_orphan_index_test.go b/pkg/dotc1z/engine/pebble/ingest_repair_orphan_index_test.go index f648e18dc..d131c294d 100644 --- a/pkg/dotc1z/engine/pebble/ingest_repair_orphan_index_test.go +++ b/pkg/dotc1z/engine/pebble/ingest_repair_orphan_index_test.go @@ -38,7 +38,7 @@ func plantOrphanPrincipalIndexEntry(t *testing.T, e *Engine, principalID, entTai func countPrincipalIndexEntries(t *testing.T, e *Engine, principalRT, principalID string) int { t.Helper() - ids, err := e.grantIdentitiesForPrincipal(context.Background(), principalRT, principalID) + ids, err := grantIdentitiesForPrincipal(context.Background(), e.db, principalRT, principalID) require.NoError(t, err) return len(ids) } diff --git a/pkg/dotc1z/engine/pebble/internal/rawdb/rawdb.go b/pkg/dotc1z/engine/pebble/internal/rawdb/rawdb.go index 9b7912357..bb4cd7883 100644 --- a/pkg/dotc1z/engine/pebble/internal/rawdb/rawdb.go +++ b/pkg/dotc1z/engine/pebble/internal/rawdb/rawdb.go @@ -15,7 +15,7 @@ // obligation unforgettable by construction. // // What this package deliberately does NOT own: the engine's write -// BARRIER (writeMu / writeWG / closing / sealed / checkpointMu) stays +// BARRIER (writeMu / the admission gate / sealed / checkpointMu) stays // in the pebble package. The barrier is lifecycle policy — who may // write when — while rawdb is write mechanics — what a write must do. // Callers arrive here already inside withWrite/withWriteAllowSealed; diff --git a/pkg/dotc1z/engine/pebble/lifecycle_lock_meta_test.go b/pkg/dotc1z/engine/pebble/lifecycle_lock_meta_test.go new file mode 100644 index 000000000..58f6c35f2 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/lifecycle_lock_meta_test.go @@ -0,0 +1,202 @@ +package pebble + +import ( + "go/ast" + "go/token" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestLifecycleMuTakersAreTransitionsOnly pins which methods acquire +// lifecycleMu. +// +// The lock order is lifecycleMu, then writeMu: EndSync holds the +// lifecycle mutex across a finalize whose steps take the write barrier. +// So any method that acquires lifecycleMu must be unreachable from inside +// a write body, or the two orders coexist and the pair deadlocks. That is +// how CurrentSyncStep became a deadlock — a method that reads like a +// getter, called from a write that wanted its own progress, on a lock +// nobody thought about at the call site. +// +// The five below are the sync-lifecycle transitions. They earn the lock +// because their bodies are read-check-write sequences over the sync-run +// record and the binding. EVERY one of them must call +// assertNotTakingLifecycleFromWrite BEFORE taking the lock: the barrier +// re-entrancy check inside the writing transitions fires at the first +// inner write — after lifecycleMu is already held — and a contended +// lifecycleMu parks the caller before that, holding the barrier a +// concurrent EndSync's finalize is waiting on. Only a guard placed +// before the lock turns that hang into a panic, so this test checks the +// position, not just the presence. +// +// A sixth taker is not forbidden, but it does have to be a decision: +// add it here, with the guard in the same shape. +func TestLifecycleMuTakersAreTransitionsOnly(t *testing.T) { + want := map[string]bool{ + "startNewSync": true, + "ResumeSync": true, + "SetCurrentSync": true, + "CheckpointSync": true, + "EndSync": true, + } + + got := map[string]bool{} + guardPos := map[string]token.Pos{} + lockPos := map[string]token.Pos{} + fset, files := parseProductionDir(t, ".") + for _, f := range files { + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + ast.Inspect(fn, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + switch sel.Sel.Name { + case "lifecycleMu": + got[fn.Name.Name] = true + if _, seen := lockPos[fn.Name.Name]; !seen { + lockPos[fn.Name.Name] = sel.Pos() + } + case "assertNotTakingLifecycleFromWrite": + if _, seen := guardPos[fn.Name.Name]; !seen { + guardPos[fn.Name.Name] = sel.Pos() + } + } + return true + }) + } + } + + require.Equal(t, want, got, + "the set of lifecycleMu takers changed. Lock order is lifecycleMu then writeMu, so a new taker "+ + "reachable from inside a write body reintroduces the deadlock TestCurrentSyncStepDoesNotDeadlockWithEndSync "+ + "covers. If the new method is a lifecycle transition, add it here; if it is a read, read the binding "+ + "instead (see CurrentSyncStep) rather than locking.") + for taker := range want { + gp, guarded := guardPos[taker] + require.True(t, guarded, + "%s takes lifecycleMu without calling assertNotTakingLifecycleFromWrite. Called from a write body it "+ + "parks on the lock holding the barrier — the deadlock — with nothing to turn the hang into a panic.", + taker) + require.Less(t, gp, lockPos[taker], + "%s calls assertNotTakingLifecycleFromWrite at %s, AFTER taking lifecycleMu at %s. Behind the lock the "+ + "guard is too late: a contended lifecycleMu parks the caller first, and the park is the deadlock.", + taker, fset.Position(gp), fset.Position(lockPos[taker])) + } +} + +// TestWriteBarrierLockedThroughOwnerPairOnly pins where writeMu may be +// locked. Ownership is recorded in lockWriteBarrier/unlockWriteBarrier, +// so a method that locks the mutex directly is invisible to the +// re-entrancy check — it hangs with no output in exactly the situation +// where nobody is looking for a lock bug, because the call reads like +// ordinary work. Close and CheckpointTo take the mutex bare on purpose: +// both run after a drain that already panicked if the caller was inside +// a write, and recording them as owners would make the teardown look +// like a write body. +func TestWriteBarrierLockedThroughOwnerPairOnly(t *testing.T) { + locksBarrier := map[string]bool{} + _, files := parseProductionDir(t, ".") + for _, f := range files { + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + ast.Inspect(fn, func(n ast.Node) bool { + if sel, ok := n.(*ast.SelectorExpr); ok && sel.Sel.Name == "writeMu" { + locksBarrier[fn.Name.Name] = true + } + return true + }) + } + } + + require.Equal(t, map[string]bool{ + "lockWriteBarrier": true, + "unlockWriteBarrier": true, + "Close": true, + "CheckpointTo": true, + }, locksBarrier, + "someone locks writeMu outside the lockWriteBarrier/unlockWriteBarrier pair. Ownership is recorded "+ + "there, so a direct lock is invisible to the re-entrancy check: go through the pair, or explain "+ + "why the new use cannot be inside a write body, the way Close and CheckpointTo do.") +} + +// TestAdmissionUsedOnlyThroughItsMethods keeps the close gate's +// invariants inside the admission type, where admission_test.go tests +// them directly. +// +// Those invariants — entering is atomic against Close's flip, the +// drains panic instead of waiting on their own caller, the teardown +// runs exactly once — hold for users of the five entry methods and for +// nobody else. Engine code that reaches past them (a bare +// admit.writers.Add, a read of admit.closing, its own drain) is taking +// on the interleaving bugs the type exists to contain, and it would do +// so silently: same package, so the compiler has no opinion. The two +// drains are additionally pinned to their single callers, because each +// encodes a lifecycle decision (shutting the gate; quiescing writes for +// a checkpoint cut) that a second call site should have to argue for +// here. +func TestAdmissionUsedOnlyThroughItsMethods(t *testing.T) { + allowedAnywhere := map[string]bool{ + "enterWrite": true, "exitWrite": true, + "enterRead": true, "exitRead": true, + "isClosing": true, + } + singleCaller := map[string]string{ + "closeAndDrain": "Close", + "drainWrites": "CheckpointTo", + } + + callers := map[string]map[string]bool{} + fset, files := parseProductionDir(t, ".") + for path, f := range files { + if filepath.Base(path) == "admission.go" { + continue + } + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + ast.Inspect(fn, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + inner, ok := sel.X.(*ast.SelectorExpr) + if !ok || inner.Sel.Name != "admit" { + return true + } + name := sel.Sel.Name + if _, pinned := singleCaller[name]; pinned { + if callers[name] == nil { + callers[name] = map[string]bool{} + } + callers[name][fn.Name.Name] = true + return true + } + require.True(t, allowedAnywhere[name], + "%s: %s reaches into the admission gate's internals (admit.%s). The gate's invariants are "+ + "only tested for its methods — go through enterWrite/exitWrite, enterRead/exitRead or "+ + "isClosing, or move the new mechanism into admission.go with a test.", + fset.Position(sel.Pos()), fn.Name.Name, name) + return true + }) + } + } + + for method, caller := range singleCaller { + require.Equal(t, map[string]bool{caller: true}, callers[method], + "admit.%s is pinned to %s. A second caller is a second place the engine decides to drain "+ + "in-flight work, which is a lifecycle decision: make it here, in this enumeration, on purpose.", + method, caller) + } +} diff --git a/pkg/dotc1z/engine/pebble/lock_checks_disabled.go b/pkg/dotc1z/engine/pebble/lock_checks_disabled.go new file mode 100644 index 000000000..a7269f132 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/lock_checks_disabled.go @@ -0,0 +1,9 @@ +//go:build !baton_lockchecks && !race + +package pebble + +// The deadlock-shape checks are compiled out: every gated branch in +// write_barrier_owner.go and admission.go is dead code under this +// constant and the tracking costs nothing. See lock_checks_enabled.go +// for what the checks are and which invocations arm them. +const writeBarrierOwnerChecks = false diff --git a/pkg/dotc1z/engine/pebble/lock_checks_enabled.go b/pkg/dotc1z/engine/pebble/lock_checks_enabled.go new file mode 100644 index 000000000..c1a6b9f67 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/lock_checks_enabled.go @@ -0,0 +1,23 @@ +//go:build baton_lockchecks || race + +package pebble + +// writeBarrierOwnerChecks turns on the deadlock-shape checks in +// write_barrier_owner.go and admission.go: barrier re-entrancy, waiting +// on your own write or pinned read, and lifecycle transitions from +// inside a write body. A compile-time constant, pebble's own invariants +// pattern, so +// the disabled build carries none of the bookkeeping — knowing which +// goroutine holds what means formatting a runtime stack (~2µs against a +// ~7µs grant write), which is the wrong trade everywhere the checks are +// not wanted: production binaries, and benchmarks, whose numbers would +// otherwise measure the check instead of the write and only on the +// Pebble side of every Pebble-vs-SQLite comparison. +// +// The race tag arms them for free in any `-race` invocation — cmd/go +// sets it automatically — so the race-based Makefile targets need no +// opt-in. Everything else gets them from -tags=baton_lockchecks, which +// `make test` and the CI workflows supply; TestLockChecksCompiledIn and +// TestLockChecksSuppliedByTestInvocations exist to make forgetting that +// loud. +const writeBarrierOwnerChecks = true diff --git a/pkg/dotc1z/engine/pebble/lock_checks_tripwire_test.go b/pkg/dotc1z/engine/pebble/lock_checks_tripwire_test.go new file mode 100644 index 000000000..77f5851d2 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/lock_checks_tripwire_test.go @@ -0,0 +1,136 @@ +package pebble + +import ( + "bufio" + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// TestLockChecksCompiledIn fails any test run that was built without the +// deadlock-shape checks. The checks are a compile-time constant +// (lock_checks_enabled.go), so nothing at runtime can report that a build +// silently dropped them — except a test that is itself compiled either +// way and looks. A plain `go test ./...` failing here is working as +// intended: the write_barrier_reentry tests don't exist in that build, +// and this failure is the only sign coverage was lost. +// +// Benchmarks are the one legitimate unarmed run — the checks would land +// on the Pebble side only of every Pebble-vs-SQLite comparison — and +// `go test -bench=. -run='^$'` selects no tests, so it never trips this. +func TestLockChecksCompiledIn(t *testing.T) { + if !writeBarrierOwnerChecks { + t.Fatal("this binary was built without the pebble deadlock-shape checks, so the tests that " + + "assert them were excluded too. Run tests with -tags=baton_lockchecks (what `make test` and CI do) " + + "or -race, which arms them for free. Only benchmarks should run unarmed: use -bench with -run='^$'.") + } +} + +// TestLockChecksSuppliedByTestInvocations is the config-level half of the +// tripwire above: it fails when a whole-tree `go test ./...` invocation +// in the Makefile or a CI workflow stops supplying -tags=baton_lockchecks +// (or -race, which arms the checks by itself). The runtime tripwire makes +// a de-armed CI run fail; this one makes the de-arming visible at the +// diff that does it, in whichever armed environment still runs. +// +// The match is line-based and deliberately dumb: a line containing both +// `go test` and `./...` must also contain the tag or -race. If an +// invocation gets split across lines or moved into a variable, update +// this to follow it — the floor assertions below fail loudly if the +// pattern stops matching anything, so a restructure cannot quietly +// retire the check. +func TestLockChecksSuppliedByTestInvocations(t *testing.T) { + root := repoRoot(t) + + var configs []string + for _, pattern := range []string{".github/workflows/*.yaml", ".github/workflows/*.yml"} { + matches, err := filepath.Glob(filepath.Join(root, pattern)) + if err != nil { + t.Fatalf("globbing workflows: %v", err) + } + configs = append(configs, matches...) + } + configs = append(configs, filepath.Join(root, "Makefile")) + + race := regexp.MustCompile(`(^|[\s=])-race([\s,]|$)`) + var violations []string + wholeTreeInvocations := map[string]int{} + for _, path := range configs { + f, err := os.Open(path) + if err != nil { + t.Fatalf("opening %s: %v", path, err) + } + // Slash-normalized, because the ".github/" prefix test below is + // the only thing keeping the workflow floor honest, and on Windows + // filepath.Rel hands back backslashes — which would leave the + // floor at zero and fail a run that has nothing wrong with it. + rel, err := filepath.Rel(root, path) + if err != nil { + t.Fatalf("relativizing %s against %s: %v", path, root, err) + } + rel = filepath.ToSlash(rel) + + scanner := bufio.NewScanner(f) + lineNo := 0 + for scanner.Scan() { + lineNo++ + line := scanner.Text() + if !strings.Contains(line, "go test") || !strings.Contains(line, "./...") { + continue + } + wholeTreeInvocations[rel]++ + if !strings.Contains(line, "baton_lockchecks") && !race.MatchString(line) { + violations = append(violations, fmt.Sprintf("%s:%d: %s", rel, lineNo, strings.TrimSpace(line))) + } + } + if err := scanner.Err(); err != nil { + t.Fatalf("reading %s: %v", path, err) + } + _ = f.Close() + } + + if len(violations) > 0 { + t.Fatalf("whole-tree `go test ./...` invocations missing -tags=baton_lockchecks (and not using -race), "+ + "so the pebble deadlock-shape checks and their tests are silently excluded there:\n %s", + strings.Join(violations, "\n ")) + } + // Floors: if a restructure moves the invocations out of reach of the + // line matcher, fail here rather than pass with nothing checked. + if wholeTreeInvocations["Makefile"] == 0 { + t.Fatal("no whole-tree `go test ./...` line found in the Makefile — if the test target changed shape, " + + "update this tripwire to follow it") + } + workflowHits := 0 + for path, n := range wholeTreeInvocations { + if strings.HasPrefix(path, ".github/") { + workflowHits += n + } + } + if workflowHits == 0 { + t.Fatal("no whole-tree `go test ./...` line found in any CI workflow — if the workflows changed shape, " + + "update this tripwire to follow it") + } +} + +// repoRoot walks up from the package directory to the module root, which +// is where the Makefile and workflows live. +func repoRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatal("no go.mod found walking up from the test's working directory") + } + dir = parent + } +} diff --git a/pkg/dotc1z/engine/pebble/lookup.go b/pkg/dotc1z/engine/pebble/lookup.go index 2b70e59fe..bc30da274 100644 --- a/pkg/dotc1z/engine/pebble/lookup.go +++ b/pkg/dotc1z/engine/pebble/lookup.go @@ -7,6 +7,8 @@ import ( "strings" "github.com/cockroachdb/pebble/v2" + + "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble/internal/rawdb" ) // Bare-id lookups. @@ -80,12 +82,17 @@ func (e *Engine) noteEntitlementKeyspaceWrite() { // entitlementIdentitiesForExternalID returns every entitlement identity // whose raw external id equals externalID, via the lazily built map. -func (e *Engine) entitlementIdentitiesForExternalID(ctx context.Context, externalID string) ([]entitlementIdentity, error) { +// +// db is the caller's admitted handle — a pinned read or the stable e.db +// of an admitted write. Every function in this resolve chain takes the +// handle rather than reaching for e.db, so one admission at the entry +// point covers the whole resolution (see pinRead). +func (e *Engine) entitlementIdentitiesForExternalID(ctx context.Context, db *rawdb.DB, externalID string) ([]entitlementIdentity, error) { gen := e.entIDLookupGen.Load() e.entIDLookupMu.Lock() defer e.entIDLookupMu.Unlock() if e.entIDLookup == nil || e.entIDLookupBuiltGen != gen { - m, err := e.buildEntitlementIDLookup(ctx) + m, err := buildEntitlementIDLookup(ctx, db) if err != nil { return nil, err } @@ -98,9 +105,9 @@ func (e *Engine) entitlementIdentitiesForExternalID(ctx context.Context, externa // buildEntitlementIDLookup scans the entitlement primary keyspace once and // groups identities by their reconstructed (== stored) external id. Only // keys are decoded; values are never touched. -func (e *Engine) buildEntitlementIDLookup(ctx context.Context) (map[string][]entitlementIdentity, error) { +func buildEntitlementIDLookup(ctx context.Context, db *rawdb.DB) (map[string][]entitlementIdentity, error) { prefix := encodeEntitlementPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -150,8 +157,8 @@ func decodeEntitlementIdentityKey(key []byte) (entitlementIdentity, bool) { // resolveEntitlementIdentityByExternalID applies the exactly-one rule to // entitlementIdentitiesForExternalID: one match wins, zero is // pebble.ErrNotFound, several is ErrAmbiguousExternalID. -func (e *Engine) resolveEntitlementIdentityByExternalID(ctx context.Context, externalID string) (entitlementIdentity, error) { - matches, err := e.entitlementIdentitiesForExternalID(ctx, externalID) +func (e *Engine) resolveEntitlementIdentityByExternalID(ctx context.Context, db *rawdb.DB, externalID string) (entitlementIdentity, error) { + matches, err := e.entitlementIdentitiesForExternalID(ctx, db, externalID) if err != nil { return entitlementIdentity{}, err } @@ -173,8 +180,8 @@ func (e *Engine) resolveEntitlementIdentityByExternalID(ctx context.Context, ext // back to direct byte-split candidates of the prefix shape, keeping a // candidate only when the grant primary keyspace actually has rows under // it. Exactly-one rule throughout. -func (e *Engine) resolveGrantScanEntitlementIdentity(ctx context.Context, entitlementID string) (entitlementIdentity, error) { - matches, err := e.entitlementIdentitiesForExternalID(ctx, entitlementID) +func (e *Engine) resolveGrantScanEntitlementIdentity(ctx context.Context, db *rawdb.DB, entitlementID string) (entitlementIdentity, error) { + matches, err := e.entitlementIdentitiesForExternalID(ctx, db, entitlementID) if err != nil { return entitlementIdentity{}, err } @@ -212,7 +219,7 @@ func (e *Engine) resolveGrantScanEntitlementIdentity(ctx context.Context, entitl stripped: true, tail: entitlementID[l+1:], } - nonEmpty, err := e.grantPrimaryPrefixNonEmpty(encodeGrantPrimaryEntitlementPrefix(cand)) + nonEmpty, err := grantPrimaryPrefixNonEmpty(db, encodeGrantPrimaryEntitlementPrefix(cand)) if err != nil { return entitlementIdentity{}, err } @@ -232,8 +239,8 @@ func (e *Engine) resolveGrantScanEntitlementIdentity(ctx context.Context, entitl } } -func (e *Engine) grantPrimaryPrefixNonEmpty(prefix []byte) (bool, error) { - iter, err := e.db.NewIter(&pebble.IterOptions{ +func grantPrimaryPrefixNonEmpty(db *rawdb.DB, prefix []byte) (bool, error) { + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -264,7 +271,7 @@ func (e *Engine) grantPrimaryPrefixNonEmpty(prefix []byte) (bool, error) { // public id also equals the query (a connector-custom stored id addresses // the row instead of the concat). Exactly one hit wins; zero is // pebble.ErrNotFound; several is ErrAmbiguousExternalID. -func (e *Engine) resolveGrantIdentityByExternalID(ctx context.Context, grantID string) (grantIdentity, error) { +func (e *Engine) resolveGrantIdentityByExternalID(ctx context.Context, db *rawdb.DB, grantID string) (grantIdentity, error) { var colons []int for i := 0; i < len(grantID); i++ { if grantID[i] == ':' { @@ -275,7 +282,7 @@ func (e *Engine) resolveGrantIdentityByExternalID(ctx context.Context, grantID s // No concat shape to split: connector-custom ids (SQLite keyed rows // by these, and provisioner revokes address grants with them) are // findable only by their STORED external id. - return e.scanGrantIdentityByStoredExternalID(ctx, grantID) + return scanGrantIdentityByStoredExternalID(ctx, db, grantID) } if len(colons) > maxBareIDColons { return grantIdentity{}, fmt.Errorf("%w: grant id has %d colons; too complex to resolve safely by string", ErrAmbiguousExternalID, len(colons)) @@ -299,7 +306,7 @@ func (e *Engine) resolveGrantIdentityByExternalID(ctx context.Context, grantID s return grantIdentity{}, err } i := colons[ii] - entMatches, err := e.entitlementIdentitiesForExternalID(ctx, grantID[:i]) + entMatches, err := e.entitlementIdentitiesForExternalID(ctx, db, grantID[:i]) if err != nil { return grantIdentity{}, err } @@ -356,7 +363,7 @@ func (e *Engine) resolveGrantIdentityByExternalID(ctx context.Context, grantID s continue } seen[string(key)] = struct{}{} - val, closer, err := e.db.Get(key) + val, closer, err := db.Get(key) if err != nil { if errors.Is(err, pebble.ErrNotFound) { continue @@ -380,7 +387,7 @@ func (e *Engine) resolveGrantIdentityByExternalID(ctx context.Context, grantID s case 0: // Every concat split missed: the id may still be a connector-custom // STORED external id that merely contains colons. - return e.scanGrantIdentityByStoredExternalID(ctx, grantID) + return scanGrantIdentityByStoredExternalID(ctx, db, grantID) case 1: return hits[0], nil default: @@ -419,8 +426,8 @@ func (e *Engine) resolveGrantIdentityByExternalID(ctx context.Context, grantID s // // Exactly-one rule: zero matches is pebble.ErrNotFound, several is // ErrAmbiguousExternalID. -func (e *Engine) scanGrantIdentityByStoredExternalID(ctx context.Context, grantID string) (grantIdentity, error) { - iter, err := e.db.NewIter(&pebble.IterOptions{ +func scanGrantIdentityByStoredExternalID(ctx context.Context, db *rawdb.DB, grantID string) (grantIdentity, error) { + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: GrantLowerBound(), UpperBound: GrantUpperBound(), }) diff --git a/pkg/dotc1z/engine/pebble/merge_surface.go b/pkg/dotc1z/engine/pebble/merge_surface.go index 6661bb59f..5e6a4d0da 100644 --- a/pkg/dotc1z/engine/pebble/merge_surface.go +++ b/pkg/dotc1z/engine/pebble/merge_surface.go @@ -20,6 +20,17 @@ package pebble // // Callers that write the entitlement keyspace through this surface // (ingests, excises) must call InvalidateBareIDLookups afterwards. +// +// ADMISSION-GATE EXCLUSION (deliberate): nothing here pins or enters +// the gate. The gate exists for callers that may overlap Close; this +// surface's one consumer (the compactor pipeline) is single-threaded +// and strictly ordered before the store's save/Close, so overlap is +// structurally impossible — and per-call admission could not cover the +// iterators and closers these methods hand out anyway, since those +// outlive the call. The e.db nil checks below are SEQUENTIAL +// post-close misuse guards (a bug in call ordering), not concurrency +// guards: if the ordering fence is violated concurrently, -race and +// pebble's own use-after-close panics are the detectors, by design. import ( "context" diff --git a/pkg/dotc1z/engine/pebble/paginate.go b/pkg/dotc1z/engine/pebble/paginate.go index bfcb862c9..7de320c9a 100644 --- a/pkg/dotc1z/engine/pebble/paginate.go +++ b/pkg/dotc1z/engine/pebble/paginate.go @@ -235,10 +235,15 @@ func (e *Engine) PaginateGrants( if limit <= 0 { limit = DefaultPageSize } + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() arena := newGrantReadArena(limit) idx := 0 prefix := encodeGrantPrefix() - records, next, err := iteratePrimaryPageWithKey(ctx, e.db, prefix, cursorBytes, limit, func() *v3.GrantRecord { + records, next, err := iteratePrimaryPageWithKey(ctx, db, prefix, cursorBytes, limit, func() *v3.GrantRecord { slot := arena.nextSlot(idx) idx++ return slot @@ -263,7 +268,12 @@ func (e *Engine) PaginateGrantsByEntitlement( if err != nil { return nil, "", err } - return iterateGrantPrimaryPage(ctx, e.db, encodeGrantPrimaryEntitlementPrefix(entID), cursorBytes, limit) + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + return iterateGrantPrimaryPage(ctx, db, encodeGrantPrimaryEntitlementPrefix(entID), cursorBytes, limit) } // PaginateGrantPrincipalKeysByEntitlement scans the primary grant keyspace under @@ -285,7 +295,12 @@ func (e *Engine) PaginateGrantPrincipalKeysByEntitlement( if err != nil { return nil, "", err } - iter, err := e.db.NewIter(&pebble.IterOptions{ + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: lower, UpperBound: upper, }) @@ -340,7 +355,12 @@ func (e *Engine) PaginateGrantsByEntitlementPrincipal( principalTypeID: principalRT, principalID: principalID, } - r, err := getGrantByIdentity(ctx, e.db, id) + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + r, err := getGrantByIdentity(ctx, db, id) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return nil, "", nil @@ -367,7 +387,12 @@ func (e *Engine) PaginateGrantsByPrincipal( if err != nil { return nil, "", err } - iter, err := e.db.NewIter(&pebble.IterOptions{ + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: lower, UpperBound: upper, }) @@ -400,7 +425,7 @@ func (e *Engine) PaginateGrantsByPrincipal( principalTypeID: principalRT, principalID: principalID, } - r, getErr := getGrantByIdentity(ctx, e.db, id) + r, getErr := getGrantByIdentity(ctx, db, id) if getErr != nil { if errors.Is(getErr, pebble.ErrNotFound) { continue @@ -439,7 +464,12 @@ func (e *Engine) PaginateGrantsByEntitlementResource( if limit <= 0 { limit = DefaultPageSize } - return iterateGrantPrimaryPage(ctx, e.db, encodeGrantPrimaryEntitlementResourcePrefix(entRT, entRID), cursorBytes, limit) + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + return iterateGrantPrimaryPage(ctx, db, encodeGrantPrimaryEntitlementResourcePrefix(entRT, entRID), cursorBytes, limit) } // PaginateGrantsByPrincipalResourceType walks the by-principal-RT @@ -460,7 +490,12 @@ func (e *Engine) PaginateGrantsByPrincipalResourceType( if err != nil { return nil, "", err } - iter, err := e.db.NewIter(&pebble.IterOptions{ + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: lower, UpperBound: upper, }) @@ -493,7 +528,7 @@ func (e *Engine) PaginateGrantsByPrincipalResourceType( principalTypeID: principalRT, principalID: components[0], } - r, getErr := getGrantByIdentity(ctx, e.db, id) + r, getErr := getGrantByIdentity(ctx, db, id) if getErr != nil { if errors.Is(getErr, pebble.ErrNotFound) { continue @@ -534,7 +569,12 @@ func (e *Engine) PaginateGrantsByNeedsExpansion( if err != nil { return nil, "", err } - iter, err := e.db.NewIter(&pebble.IterOptions{ + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: lower, UpperBound: upper, }) @@ -567,7 +607,7 @@ func (e *Engine) PaginateGrantsByNeedsExpansion( principalTypeID: components[4], principalID: components[5], } - r, getErr := getGrantByIdentity(ctx, e.db, id) + r, getErr := getGrantByIdentity(ctx, db, id) if getErr != nil { if errors.Is(getErr, pebble.ErrNotFound) { continue @@ -598,8 +638,13 @@ func (e *Engine) PaginateResources( if err != nil { return nil, "", err } + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() prefix := encodeResourcePrefix() - return iteratePrimaryPageWithKey(ctx, e.db, prefix, cursorBytes, limit, func() *v3.ResourceRecord { + return iteratePrimaryPageWithKey(ctx, db, prefix, cursorBytes, limit, func() *v3.ResourceRecord { return &v3.ResourceRecord{} }) } @@ -620,7 +665,12 @@ func (e *Engine) PaginateResourcesByParent( if err != nil { return nil, "", err } - iter, err := e.db.NewIter(&pebble.IterOptions{ + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: lower, UpperBound: upper, }) @@ -643,7 +693,7 @@ func (e *Engine) PaginateResourcesByParent( if !ok { continue } - val, closer, getErr := e.db.Get(encodeResourceKey(childRT, childID)) + val, closer, getErr := db.Get(encodeResourceKey(childRT, childID)) if getErr != nil { if errors.Is(getErr, pebble.ErrNotFound) { continue @@ -677,8 +727,13 @@ func (e *Engine) PaginateResourceTypes( if err != nil { return nil, "", err } + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() prefix := encodeResourceTypePrefix() - return iteratePrimaryPageWithKey(ctx, e.db, prefix, cursorBytes, limit, func() *v3.ResourceTypeRecord { + return iteratePrimaryPageWithKey(ctx, db, prefix, cursorBytes, limit, func() *v3.ResourceTypeRecord { return &v3.ResourceTypeRecord{} }) } @@ -692,8 +747,13 @@ func (e *Engine) PaginateEntitlements( if err != nil { return nil, "", err } + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() prefix := encodeEntitlementPrefix() - return iteratePrimaryPageWithKey(ctx, e.db, prefix, cursorBytes, limit, func() *v3.EntitlementRecord { + return iteratePrimaryPageWithKey(ctx, db, prefix, cursorBytes, limit, func() *v3.EntitlementRecord { return &v3.EntitlementRecord{} }) } @@ -714,7 +774,12 @@ func (e *Engine) PaginateEntitlementsByResource( if err != nil { return nil, "", err } - iter, err := e.db.NewIter(&pebble.IterOptions{ + db, release, err := e.pinRead() + if err != nil { + return nil, "", err + } + defer release() + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: lower, UpperBound: upper, }) diff --git a/pkg/dotc1z/engine/pebble/paginate_close_lifecycle_test.go b/pkg/dotc1z/engine/pebble/paginate_close_lifecycle_test.go new file mode 100644 index 000000000..7c835ea21 --- /dev/null +++ b/pkg/dotc1z/engine/pebble/paginate_close_lifecycle_test.go @@ -0,0 +1,553 @@ +package pebble + +import ( + "context" + "errors" + "fmt" + "go/ast" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + v3 "github.com/conductorone/baton-sdk/pb/c1/storage/v3" + "github.com/conductorone/baton-sdk/pkg/connectorstore" + "github.com/conductorone/baton-sdk/pkg/types/sessions" +) + +// callAfterClose runs fn and converts a panic into an error. +// +// The methods under test are being checked precisely because they may +// dereference a handle Close nil'd. Left unrecovered, the first one to +// do so takes the package's test binary down and every other verdict in +// this file is lost with it — the run reports one panic instead of the +// list of entry points that need the guard. +func callAfterClose(fn func() error) (err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("panicked instead of returning an error: %v", r) + } + }() + return fn() +} + +// TestReadSurfaceAfterCloseReturnsClosing pins the lifecycle contract on +// the paginate, iterate, and point-read surfaces. +// +// The Engine doc states it outright: "After Close, all methods return +// ErrEngineClosing." Before pinRead these families reached +// rawdb.DB.NewIter on a nil receiver instead — the paginate methods had +// no guard, and the Iterate family had neither a guard nor the nil check +// the invariant-scan surface carried +// (TestIngestScanSurfaceAfterCloseReturnsClosing covers that one). +// +// The point reads are here because "all methods" is the contract and the +// scan families are only the part that crashed loudly. A Get that skips +// the pin is the same use-after-close with a quieter failure — a stale +// handle answers, or Close returns while the read is still on it — and +// nothing about the two-line body invites a second look. +// +// Each entry point is its own subtest so one run names every method that +// regressed rather than stopping at the first. +func TestReadSurfaceAfterCloseReturnsClosing(t *testing.T) { + ctx := context.Background() + a := newAdapter(t) + _, err := a.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + e := a.PebbleEngine() + require.NoError(t, a.PutGrants(ctx, mkV2Grant("", "ent-A", "user", "alice"))) + require.NoError(t, e.Close()) + + entID := entitlementIdentityFromParts("app", "github", canonicalTestEntID("ent-A")) + + for _, tc := range []struct { + name string + call func() error + }{ + {"PaginateGrants", func() error { + _, _, err := e.PaginateGrants(ctx, "", 10) + return err + }}, + {"PaginateGrantsByEntitlement", func() error { + _, _, err := e.PaginateGrantsByEntitlement(ctx, entID, "", 10) + return err + }}, + {"PaginateGrantPrincipalKeysByEntitlement", func() error { + _, _, err := e.PaginateGrantPrincipalKeysByEntitlement(ctx, entID, "", 10) + return err + }}, + {"PaginateGrantsByEntitlementPrincipal", func() error { + _, _, err := e.PaginateGrantsByEntitlementPrincipal(ctx, entID, "user", "alice", "", 10) + return err + }}, + {"PaginateGrantsByPrincipal", func() error { + _, _, err := e.PaginateGrantsByPrincipal(ctx, "user", "alice", "", 10) + return err + }}, + {"PaginateGrantsByEntitlementResource", func() error { + _, _, err := e.PaginateGrantsByEntitlementResource(ctx, "app", "github", "", 10) + return err + }}, + {"PaginateGrantsByPrincipalResourceType", func() error { + _, _, err := e.PaginateGrantsByPrincipalResourceType(ctx, "user", "", 10) + return err + }}, + {"PaginateGrantsByNeedsExpansion", func() error { + _, _, err := e.PaginateGrantsByNeedsExpansion(ctx, "", 10) + return err + }}, + {"PaginateResources", func() error { + _, _, err := e.PaginateResources(ctx, "", 10) + return err + }}, + {"PaginateResourcesByParent", func() error { + _, _, err := e.PaginateResourcesByParent(ctx, "app", "github", "", 10) + return err + }}, + {"PaginateResourceTypes", func() error { + _, _, err := e.PaginateResourceTypes(ctx, "", 10) + return err + }}, + {"PaginateEntitlements", func() error { + _, _, err := e.PaginateEntitlements(ctx, "", 10) + return err + }}, + {"PaginateEntitlementsByResource", func() error { + _, _, err := e.PaginateEntitlementsByResource(ctx, "app", "github", "", 10) + return err + }}, + {"IterateGrants", func() error { + return e.IterateGrants(ctx, func(*v3.GrantRecord) bool { return true }) + }}, + {"IterateGrantsByEntitlement", func() error { + return e.IterateGrantsByEntitlement(ctx, canonicalTestEntID("ent-A"), func(*v3.GrantRecord) bool { return true }) + }}, + {"IterateGrantsByPrincipal", func() error { + return e.IterateGrantsByPrincipal(ctx, "user", "alice", func(*v3.GrantRecord) bool { return true }) + }}, + {"IterateGrantsByPrincipalResourceType", func() error { + return e.IterateGrantsByPrincipalResourceType(ctx, "user", func(*v3.GrantRecord) bool { return true }) + }}, + {"IterateGrantsByNeedsExpansion", func() error { + return e.IterateGrantsByNeedsExpansion(ctx, func(*v3.GrantRecord) bool { return true }) + }}, + {"IterateGrantsByEntitlementBucket", func() error { + return e.IterateGrantsByEntitlementBucket(ctx, entID, DigestBucket{}, func(*v3.GrantRecord) bool { return true }) + }}, + {"IterateResources", func() error { + return e.IterateResources(ctx, func(*v3.ResourceRecord) bool { return true }) + }}, + {"IterateResourcesByParent", func() error { + return e.IterateResourcesByParent(ctx, "app", "github", func(*v3.ResourceRecord) bool { return true }) + }}, + {"IterateResourceTypes", func() error { + return e.IterateResourceTypes(ctx, func(*v3.ResourceTypeRecord) bool { return true }) + }}, + {"IterateEntitlements", func() error { + return e.IterateEntitlements(ctx, func(*v3.EntitlementRecord) bool { return true }) + }}, + {"IterateEntitlementsByResource", func() error { + return e.IterateEntitlementsByResource(ctx, "app", "github", func(*v3.EntitlementRecord) bool { return true }) + }}, + {"IterateAssets", func() error { + return e.IterateAssets(ctx, func(*v3.AssetRecord) bool { return true }) + }}, + {"IterateAllSyncRuns", func() error { + return e.IterateAllSyncRuns(ctx, func(*v3.SyncRunRecord) bool { return true }) + }}, + + // Point reads. A single Get touches the same closed handle a scan + // does, and it reaches it by a shorter path — no iterator, so + // nothing on the way that happens to check. These take arguments + // that match nothing on purpose: the answer under test is the + // lifecycle error, and a lookup that would have missed anyway is + // the case where a missing guard is easiest to mistake for + // working code. + {"GetGrantRecord", func() error { + _, err := e.GetGrantRecord(ctx, "no-such-grant") + return err + }}, + {"GetEntitlementRecord", func() error { + _, err := e.GetEntitlementRecord(ctx, "no-such-entitlement") + return err + }}, + {"GetResourceRecord", func() error { + _, err := e.GetResourceRecord(ctx, "app", "github") + return err + }}, + {"GetResourceTypeRecord", func() error { + _, err := e.GetResourceTypeRecord(ctx, "app") + return err + }}, + {"GetAssetRecord", func() error { + _, err := e.GetAssetRecord(ctx, "no-such-asset") + return err + }}, + {"GetSyncRunRecord", func() error { + _, err := e.GetSyncRunRecord(ctx, "no-such-sync") + return err + }}, + {"HasResourceRecord", func() error { + _, err := e.HasResourceRecord(ctx, "app", "github") + return err + }}, + {"GetEntitlementDigestRoot", func() error { + _, _, err := e.GetEntitlementDigestRoot(ctx, entID) + return err + }}, + {"GetGrantDigestGlobalRoot", func() error { + _, _, err := e.GetGrantDigestGlobalRoot(ctx) + return err + }}, + {"ComputeEntitlementBucketDigest", func() error { + _, _, err := e.ComputeEntitlementBucketDigest(ctx, entID, DigestBucket{}) + return err + }}, + {"GetEntitlementGrantDigestNodes", func() error { + _, _, err := e.GetEntitlementGrantDigestNodes(ctx, mkV2Entitlement("ent-A", "app", "github"), 0) + return err + }}, + // The session reads validate their option bag first and reject a + // missing sync id, so they need one supplied explicitly to reach + // the lifecycle check at all. The engine is closed and the id + // names no session; neither matters to what is under test. + {"SessionGet", func() error { + _, _, err := e.SessionGet(ctx, "no-such-key", sessions.WithSyncID("no-such-sync")) + return err + }}, + {"SessionGetMany", func() error { + _, _, err := e.SessionGetMany(ctx, []string{"no-such-key"}, sessions.WithSyncID("no-such-sync")) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + require.ErrorIs(t, callAfterClose(tc.call), ErrEngineClosing) + }) + } +} + +// pinnedReadPrefixes are the exported read families that must go +// through pinRead. Every method whose name starts with one of these is +// a self-contained scan: it opens an iterator, drains it, and closes it +// before returning, so a pin scoped to the call covers the whole read. +// +// Surfaces that hand a live handle back to the caller — merge_surface's +// NewIter and Get return an iterator and a closer the caller uses after +// the call returns — are deliberately absent. A call-scoped pin would +// release while the caller still holds the handle, so they need the +// release tied to the returned object instead, and listing them here +// would let a useless pin satisfy the check. +var pinnedReadPrefixes = []string{"Paginate", "Iterate", "ForEach"} + +// TestCloseWaitsForInFlightAdmission pins the ordering that keeps a +// joiner's closing check and its counter increment from straddling +// Close's flip. +// +// Re-checking the flag after the increment does not close that window, +// it just narrows it: a reader that read closing==false can be +// descheduled and land its increment after Close's drain sampled the +// counter at zero — a member the drain never counted, so the teardown +// runs under a live read. It surfaces as a rare crash under load, on +// the interleaving TestConcurrentCloseWithPaginatedReads spends four +// goroutines trying to hit, which is the worst way to learn about it. +// +// Holding the admission lock the way pinRead does is the whole +// interleaving, stopped in the middle: while a pin is being acquired, +// Close must not have flipped the flag yet. +func TestCloseWaitsForInFlightAdmission(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + _, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + // A pin caught between its closing check and its increment: hold + // the gate's admission lock the way enterRead does, with the + // increment not yet issued. White-box into the admission type — the + // interleaving under test is the two statements inside enterRead, + // so no method can stand in for the stopped middle. + e.admit.mu.RLock() + + closed := make(chan error, 1) + go func() { closed <- e.Close() }() + + require.Never(t, func() bool { return e.admit.isClosing() }, 200*time.Millisecond, 5*time.Millisecond, + "Close flipped the closing flag while a pin was mid-acquisition. That pin's increment now lands on a "+ + "drain that already sampled zero, so the teardown runs under a live read instead of refusing it") + + // Finish acquiring, then let Close proceed: it must now see the pin + // and wait for it rather than tear the handle down underneath it. + e.admit.countMu.Lock() + e.admit.readers++ + e.admit.countMu.Unlock() + e.admit.mu.RUnlock() + + require.Never(t, func() bool { return e.db == nil }, 100*time.Millisecond, 5*time.Millisecond, + "Close tore the handle down while a read was pinned") + // exitRead decrements and broadcasts the drain awake; the untracked + // membership entry is a no-op to release. + e.admit.exitRead() + require.NoError(t, <-closed) +} + +// TestScanReadsArePinned keeps the read surface on the pin. +// +// pinRead is what orders a read's view of the handle against Close's +// teardown and what makes Close wait for the read to finish. A method +// that reaches for e.db directly is back to borrowing a handle the +// teardown can pull out from under it, and it fails as a panic from +// inside pebble rather than as an error. +// +// The check is mechanical because the mistake is invisible at the call +// site: a direct field read compiles, passes every functional test, and +// only surfaces when a Close lands mid-scan. +func TestScanReadsArePinned(t *testing.T) { + _, files := parseProductionDir(t, ".") + + pinnedFamily := func(name string) bool { + for _, prefix := range pinnedReadPrefixes { + if strings.HasPrefix(name, prefix) { + return true + } + } + return false + } + + // Scoped to methods on *Engine. The same names exist as free + // functions in merge_accessor.go (ForEachGrantIndexKey and friends), + // which encode index keys from a record and never touch the handle — + // there is nothing there to pin. + onEngine := func(fn *ast.FuncDecl) bool { + if fn.Recv == nil || len(fn.Recv.List) != 1 { + return false + } + star, ok := fn.Recv.List[0].Type.(*ast.StarExpr) + if !ok { + return false + } + ident, ok := star.X.(*ast.Ident) + return ok && ident.Name == "Engine" + } + + checked := 0 + for _, f := range files { + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || !onEngine(fn) || !pinnedFamily(fn.Name.Name) { + continue + } + checked++ + t.Run(fn.Name.Name, func(t *testing.T) { + var pinned, direct bool + ast.Inspect(fn, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + switch sel.Sel.Name { + case "pinRead": + pinned = true + case "db": + if ident, ok := sel.X.(*ast.Ident); ok && ident.Name == "e" { + direct = true + } + } + return true + }) + require.True(t, pinned, + "%s reads without pinning the handle: call e.pinRead, defer its release, and read through the handle it returns", fn.Name.Name) + require.False(t, direct, + "%s reads e.db directly. The pinned handle is the one Close's drain accounts for; re-reading the "+ + "field inside the body is the unordered access pinRead exists to remove", fn.Name.Name) + + hasLoop, loopChecksCtx := scanLoopCancellation(fn) + if hasLoop { + require.True(t, loopChecksCtx, + "%s scans without checking ctx.Err() in the loop. The pin makes Close wait for this scan, so a "+ + "full-keyspace read — or a yield callback that blocks on IO — holds the teardown open for as "+ + "long as it takes, with no way for the caller to call it off", fn.Name.Name) + } + }) + } + } + require.Positive(t, checked, "no read methods matched %v: this check has drifted off the surface it is meant to hold", pinnedReadPrefixes) +} + +// scanLoopCancellation reports whether fn drives a pebble iterator +// directly, and whether any such loop consults ctx.Err(). +// +// Keyed on an iterator loop rather than on any loop at all, because the +// unbounded thing is the keyspace, not the ranging. PaginateGrants +// ranges over the page it just read to reconcile absent fields; that +// loop is bounded by the page limit and the scan feeding it lives in a +// shared helper that checks per iteration. Requiring a check there +// would be noise, and noise is how a meta-test gets an exemption list +// and stops meaning anything. +// +// Two loop shapes count as iterator loops: a `for` whose condition +// calls Valid() (`for iter.First(); iter.Valid(); iter.Next()`), and +// the seek-driven shape whose condition is a bool fed by First/SeekGE +// (`for valid := iter.First(); valid;` — the distinct-referent scans in +// ingest_facts/ingest_repair). The second used to be invisible here, +// which meant those scans could lose their ctx check without any test +// noticing. +// +// Deliberately "any iterator loop" rather than "every" one: these shapes +// nest, an outer index walk feeding an inner primary-key fetch, and one +// check per scan is what bounds the pin. +func scanLoopCancellation(fn *ast.FuncDecl) (bool, bool) { + var hasLoop, checksCtx bool + callsMethod := func(node ast.Node, names ...string) bool { + if node == nil { + return false + } + found := false + ast.Inspect(node, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + for _, name := range names { + if sel.Sel.Name == name { + found = true + } + } + } + return true + }) + return found + } + isIterLoop := func(loop *ast.ForStmt) bool { + if callsMethod(loop.Cond, "Valid") { + return true + } + // Seek-driven: `for valid := iter.First(); valid;` — condition + // is a bare bool whose init assigns from a positioning call. + if _, ok := loop.Cond.(*ast.Ident); ok && callsMethod(loop.Init, "First", "SeekGE", "SeekLT", "Last") { + return true + } + return false + } + ast.Inspect(fn, func(n ast.Node) bool { + loop, ok := n.(*ast.ForStmt) + if !ok || !isIterLoop(loop) { + return true + } + hasLoop = true + if callsMethod(loop.Body, "Err") { + checksCtx = true + } + return true + }) + return hasLoop, checksCtx +} + +// TestConcurrentCloseWithPaginatedReads is the concurrent half of the +// same contract: "Concurrent Reader/Writer calls are safe." +// +// Writers are held to it by a real barrier — the closing check, gate +// admission, and the barrier mutex. Readers hold only a pin: gate +// admission with no mutual exclusion, so this hammers the pin's whole +// job — a read admitted before Close's flip must complete against a +// live handle, and one arriving after must be refused, across every +// interleaving four goroutines can produce. +// +// A reader here may only succeed or be refused with ErrEngineClosing. +// A panic, any other error, or a race report is a failure. +func TestConcurrentCloseWithPaginatedReads(t *testing.T) { + ctx := context.Background() + a := newAdapter(t) + _, err := a.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + e := a.PebbleEngine() + + const ( + readers = 4 + grantsPerUser = 256 + ) + principals := make([]string, 0, readers) + grants := make([]*v2.Grant, 0, readers*grantsPerUser) + for w := range readers { + principal := fmt.Sprintf("user%02d", w) + principals = append(principals, principal) + for i := range grantsPerUser { + grants = append(grants, mkV2Grant("", fmt.Sprintf("ent-%02d-%05d", w, i), "user", principal)) + } + } + require.NoError(t, a.PutGrants(ctx, grants...)) + + var ( + wg sync.WaitGroup + failuresMu sync.Mutex + failures []string + pages atomic.Int64 + refusals atomic.Int64 + ) + failf := func(format string, args ...any) { + failuresMu.Lock() + defer failuresMu.Unlock() + failures = append(failures, fmt.Sprintf(format, args...)) + } + + // Readers exit on a deadline as well as on the close boundary. One + // that only stopped when it observed Close would hang forever + // against a fix that drains readers before tearing the handle down, + // turning this probe into a deadlock trap for the very change it is + // asking for. + deadline := time.Now().Add(5 * time.Second) + + // Both read shapes are hammered: the cursor-paged one and the + // callback scan. They pin identically, but the scan holds its + // iterator across the whole keyspace rather than one page, so it is + // the one with a real chance of still being inside pebble when the + // teardown starts. + for i, principal := range principals { + scan := i%2 == 1 + wg.Add(1) + go func() { + defer wg.Done() + defer func() { + if r := recover(); r != nil { + failf("reader %s panicked mid-read: %v", principal, r) + } + }() + for time.Now().Before(deadline) { + var err error + if scan { + err = e.IterateGrantsByPrincipal(ctx, "user", principal, func(*v3.GrantRecord) bool { return true }) + } else { + _, _, err = e.PaginateGrantsByPrincipal(ctx, "user", principal, "", 32) + } + if err != nil { + if !errors.Is(err, ErrEngineClosing) { + failf("reader %s: read across Close returned %v, want ErrEngineClosing", principal, err) + } + refusals.Add(1) + return + } + pages.Add(1) + } + }() + } + + // Let the readers build pressure, then close under them. + time.Sleep(50 * time.Millisecond) + closeErr := e.Close() + wg.Wait() + + if len(failures) > 0 { + for _, f := range failures { + t.Error(f) + } + t.FailNow() + } + require.NoError(t, closeErr, "Close failed with reads in flight") + require.Positive(t, pages.Load(), + "no reader completed a page before Close; the race window was never exercised") + require.Positive(t, refusals.Load(), + "no reader was refused at the close boundary; every reader exited on its deadline instead, so the window was never exercised") +} diff --git a/pkg/dotc1z/engine/pebble/production_bench_test.go b/pkg/dotc1z/engine/pebble/production_bench_test.go index 584ff09b0..33d3cc9af 100644 --- a/pkg/dotc1z/engine/pebble/production_bench_test.go +++ b/pkg/dotc1z/engine/pebble/production_bench_test.go @@ -15,6 +15,15 @@ import ( "github.com/conductorone/baton-sdk/pkg/dotc1z/c1zstore" ) +// These benchmarks measure the production write path as long as the +// binary is built without -tags=baton_lockchecks and without -race: +// the deadlock-shape checks (see lock_checks_enabled.go) are a +// compile-time constant, so a plain `go test -bench` carries none of +// their cost. An armed build formats a runtime stack per barrier +// acquisition — about 2µs against roughly 7µs for the grant write being +// measured, and only on the Pebble side of every Pebble-vs-SQLite +// comparison here — so don't read numbers from one. + func benchmarkGrants(n int) []*v2.Grant { grants := make([]*v2.Grant, 0, n) for i := 0; i < n; i++ { diff --git a/pkg/dotc1z/engine/pebble/resource_types.go b/pkg/dotc1z/engine/pebble/resource_types.go index bbef3af55..565b642bd 100644 --- a/pkg/dotc1z/engine/pebble/resource_types.go +++ b/pkg/dotc1z/engine/pebble/resource_types.go @@ -54,8 +54,13 @@ func (e *Engine) PutResourceTypeRecords(ctx context.Context, records ...*v3.Reso } func (e *Engine) GetResourceTypeRecord(ctx context.Context, externalID string) (*v3.ResourceTypeRecord, error) { + db, release, err := e.pinRead() + if err != nil { + return nil, err + } + defer release() key := encodeResourceTypeKey(externalID) - val, closer, err := e.db.Get(key) + val, closer, err := db.Get(key) if err != nil { return nil, err } @@ -82,8 +87,13 @@ func (e *Engine) DeleteResourceTypeRecord(ctx context.Context, externalID string } func (e *Engine) IterateResourceTypes(ctx context.Context, yield func(*v3.ResourceTypeRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() prefix := encodeResourceTypePrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -92,6 +102,9 @@ func (e *Engine) IterateResourceTypes(ctx context.Context, yield func(*v3.Resour } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } r := &v3.ResourceTypeRecord{} if err := unmarshalRecord(iter.Value(), r); err != nil { return fmt.Errorf("iterate resource_types: %w", err) diff --git a/pkg/dotc1z/engine/pebble/resources.go b/pkg/dotc1z/engine/pebble/resources.go index b05b5ca59..532b7a737 100644 --- a/pkg/dotc1z/engine/pebble/resources.go +++ b/pkg/dotc1z/engine/pebble/resources.go @@ -111,8 +111,13 @@ func (e *Engine) PutResourceRecords(ctx context.Context, records ...*v3.Resource } func (e *Engine) GetResourceRecord(ctx context.Context, resourceTypeID, resourceID string) (*v3.ResourceRecord, error) { + db, release, err := e.pinRead() + if err != nil { + return nil, err + } + defer release() key := encodeResourceKey(resourceTypeID, resourceID) - val, closer, err := e.db.Get(key) + val, closer, err := db.Get(key) if err != nil { return nil, err } @@ -150,8 +155,13 @@ func (e *Engine) DeleteResourceRecord(ctx context.Context, resourceTypeID, resou } func (e *Engine) IterateResources(ctx context.Context, yield func(*v3.ResourceRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() prefix := encodeResourcePrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -160,6 +170,9 @@ func (e *Engine) IterateResources(ctx context.Context, yield func(*v3.ResourceRe } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } r := &v3.ResourceRecord{} if err := unmarshalRecord(iter.Value(), r); err != nil { return fmt.Errorf("iterate resources: %w", err) @@ -172,8 +185,13 @@ func (e *Engine) IterateResources(ctx context.Context, yield func(*v3.ResourceRe } func (e *Engine) IterateResourcesByParent(ctx context.Context, parentRT, parentID string, yield func(*v3.ResourceRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() indexPrefix := encodeResourceByParentPrefix(parentRT, parentID) - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: indexPrefix, UpperBound: upperBoundOf(indexPrefix), }) @@ -182,12 +200,15 @@ func (e *Engine) IterateResourcesByParent(ctx context.Context, parentRT, parentI } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } // Decode (childRT, childID) from the tail. childRT, childID, ok := decodeTwoTupleComponents(iter.Key(), indexPrefix) if !ok { continue } - val, closer, err := e.db.Get(encodeResourceKey(childRT, childID)) + val, closer, err := db.Get(encodeResourceKey(childRT, childID)) if err != nil { if errors.Is(err, pebble.ErrNotFound) { continue diff --git a/pkg/dotc1z/engine/pebble/session_store.go b/pkg/dotc1z/engine/pebble/session_store.go index 9ba83d154..a230b2a07 100644 --- a/pkg/dotc1z/engine/pebble/session_store.go +++ b/pkg/dotc1z/engine/pebble/session_store.go @@ -55,8 +55,13 @@ func (e *Engine) SessionGet(ctx context.Context, key string, opt ...sessions.Ses return nil, false, fmt.Errorf("error applying session option: %w", err) } + db, release, err := e.pinRead() + if err != nil { + return nil, false, err + } + defer release() keyBytes := encodeSessionKey(bag.SyncID, bag.Prefix+key) - val, closer, err := e.db.Get(keyBytes) + val, closer, err := db.Get(keyBytes) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return nil, false, nil @@ -102,9 +107,14 @@ func (e *Engine) SessionGetMany(ctx context.Context, keys []string, opt ...sessi results := make([]item, 0, len(keys)) messageSize := 0 + db, release, err := e.pinRead() + if err != nil { + return nil, nil, err + } + defer release() for _, prefixedKey := range prefixedKeys { keyBytes := encodeSessionKey(bag.SyncID, prefixedKey) - val, closer, err := e.db.Get(keyBytes) + val, closer, err := db.Get(keyBytes) if err != nil { if errors.Is(err, pebble.ErrNotFound) { continue @@ -196,7 +206,12 @@ func (e *Engine) sessionGetAllChunk(ctx context.Context, pageToken string, sizeL default: lower = syncPrefix } - iter, err := e.db.NewIter(&pebble.IterOptions{ + db, release, err := e.pinRead() + if err != nil { + return nil, "", 0, err + } + defer release() + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: lower, UpperBound: upperBoundOf(syncPrefix), }) @@ -272,7 +287,7 @@ func (e *Engine) SessionSet(ctx context.Context, key string, value []byte, opt . } // Under the write barrier like every other record write: a bare Set - // would race Close's teardown (no writeWG coverage) and could land + // would race Close's teardown (no close-drain coverage) and could land // inside CheckpointTo's Flush→Checkpoint window as a WAL-only record // the truncate silently drops from the saved snapshot. // diff --git a/pkg/dotc1z/engine/pebble/sync_runs.go b/pkg/dotc1z/engine/pebble/sync_runs.go index e87c58d61..fc628f8f6 100644 --- a/pkg/dotc1z/engine/pebble/sync_runs.go +++ b/pkg/dotc1z/engine/pebble/sync_runs.go @@ -46,7 +46,16 @@ func (e *Engine) GetSyncRunRecord(ctx context.Context, syncID string) (*v3.SyncR if syncID == "" { return nil, errors.New("GetSyncRunRecord: empty sync_id") } - val, closer, err := e.db.Get(encodeSyncRunKey()) + // Pinned even though several callers are already-admitted writes: + // nesting a read entry inside a write entry is safe (the gate counts + // entries, it doesn't own them), and the pin is what protects the + // bare-Engine callers (stats, sync-meta, clone). + db, release, err := e.pinRead() + if err != nil { + return nil, err + } + defer release() + val, closer, err := db.Get(encodeSyncRunKey()) if err != nil { return nil, err } @@ -98,7 +107,9 @@ func (e *Engine) DeleteSyncRunRecord(ctx context.Context, syncID string) error { // hasSyncRun reports whether the engine already holds a sync-run // record (the file's one sync). StartNewSync uses it to decide whether -// a prior sync must be wiped before the replacement is bound. +// a prior sync must be wiped before the replacement is bound. Its only +// caller runs as an admitted write (startNewSync holds gate admission), +// so the bare e.db read here cannot race the teardown. func (e *Engine) hasSyncRun() (bool, error) { _, closer, err := e.db.Get(encodeSyncRunKey()) if err != nil { @@ -114,8 +125,13 @@ func (e *Engine) hasSyncRun() (bool, error) { // IterateAllSyncRuns iterates every sync_run record in the engine. // Used by callers that want "what syncs do I have available?". func (e *Engine) IterateAllSyncRuns(ctx context.Context, yield func(*v3.SyncRunRecord) bool) error { + db, release, err := e.pinRead() + if err != nil { + return err + } + defer release() prefix := encodeSyncRunFullPrefix() - iter, err := e.db.NewIter(&pebble.IterOptions{ + iter, err := db.NewIter(&pebble.IterOptions{ LowerBound: prefix, UpperBound: upperBoundOf(prefix), }) @@ -124,6 +140,9 @@ func (e *Engine) IterateAllSyncRuns(ctx context.Context, yield func(*v3.SyncRunR } defer iter.Close() for iter.First(); iter.Valid(); iter.Next() { + if err := ctx.Err(); err != nil { + return err + } r := &v3.SyncRunRecord{} if err := unmarshalRecord(iter.Value(), r); err != nil { return fmt.Errorf("iterate sync_runs: %w", err) diff --git a/pkg/dotc1z/engine/pebble/sync_stats_sidecar.go b/pkg/dotc1z/engine/pebble/sync_stats_sidecar.go index 20f71377f..f50ea15e5 100644 --- a/pkg/dotc1z/engine/pebble/sync_stats_sidecar.go +++ b/pkg/dotc1z/engine/pebble/sync_stats_sidecar.go @@ -61,7 +61,12 @@ func SyncStatsSidecarUpperBound() []byte { // mismatched id misses); pass "" to read whatever is stored. Errors // surface for real read failures only. func (e *Engine) readSyncStats(ctx context.Context, syncID string) (*v3.SyncStatsRecord, error) { - val, closer, err := e.db.Get(encodeSyncStatsKey()) + db, release, err := e.pinRead() + if err != nil { + return nil, err + } + defer release() + val, closer, err := db.Get(encodeSyncStatsKey()) if err != nil { if errors.Is(err, pebble.ErrNotFound) { return nil, nil @@ -90,8 +95,8 @@ func (e *Engine) writeSyncStats(ctx context.Context, rec *v3.SyncStatsRecord) er } // AllowSealed under the write barrier: callers span EndSync's sealed // finalize window and the compactor's bound-sync flow. The barrier - // (rather than a bare Set) gives the write the closing check, writeWG - // coverage against Close's teardown, and exclusion from CheckpointTo's + // (rather than a bare Set) gives the write the closing check, close- + // drain coverage against Close's teardown, and exclusion from CheckpointTo's // Flush→Checkpoint window (a WAL-only record landing mid-window would // be truncated out of the saved snapshot). return e.withWriteAllowSealed(func() error { @@ -111,10 +116,17 @@ func (e *Engine) writeSyncStats(ctx context.Context, rec *v3.SyncStatsRecord) er // field removed the final stats-sidecar full unmarshal from compaction; in the // same-size syncs=50 overlay case allocs dropped from ~3.05M to ~2.41M/op. func (e *Engine) computeSyncStats(ctx context.Context, syncID string) (*v3.SyncStatsRecord, error) { + // One pin spans all five scans: the Stats reader path calls this on + // a bare Engine, and each scan below reads the handle. + db, release, err := e.pinRead() + if err != nil { + return nil, err + } + defer release() rec := &v3.SyncStatsRecord{} rec.SetSyncId(syncID) - resourceTypes, err := countKeyRange(ctx, e.db, ResourceTypeLowerBound(), ResourceTypeUpperBound(), nil) + resourceTypes, err := countKeyRange(ctx, db, ResourceTypeLowerBound(), ResourceTypeUpperBound(), nil) if err != nil { return nil, fmt.Errorf("computeSyncStats: resource_types: %w", err) } @@ -129,7 +141,7 @@ func (e *Engine) computeSyncStats(ctx context.Context, syncID string) (*v3.SyncS resLower := ResourceLowerBound() var rtScratch, curRT []byte var curCount int64 - resources, err := countKeyRange(ctx, e.db, resLower, ResourceUpperBound(), func(key []byte, _ []byte) error { + resources, err := countKeyRange(ctx, db, resLower, ResourceUpperBound(), func(key []byte, _ []byte) error { if len(key) <= len(resLower) { return fmt.Errorf("resource key shorter than expected lower bound") } @@ -163,7 +175,7 @@ func (e *Engine) computeSyncStats(ctx context.Context, syncID string) (*v3.SyncS // time each resource type appears (same shape as the grant grouping // below). entitlementsByRTPtr := map[string]*int64{} - entitlements, err := countKeyRange(ctx, e.db, EntitlementLowerBound(), EntitlementUpperBound(), func(_ []byte, value []byte) error { + entitlements, err := countKeyRange(ctx, db, EntitlementLowerBound(), EntitlementUpperBound(), func(_ []byte, value []byte) error { rt, err := scanEntitlementResourceTypeRaw(value) if err != nil { return err @@ -201,7 +213,7 @@ func (e *Engine) computeSyncStats(ctx context.Context, syncID string) (*v3.SyncS grantsByEntitlementRT = ds.grantsByEntitlementRT } else { grantsByEntRTPtr := map[string]*int64{} - grants, err = countKeyRange(ctx, e.db, GrantLowerBound(), GrantUpperBound(), func(_ []byte, value []byte) error { + grants, err = countKeyRange(ctx, db, GrantLowerBound(), GrantUpperBound(), func(_ []byte, value []byte) error { entRT, err := scanGrantEntitlementResourceTypeRaw(value) if err != nil { return err @@ -224,7 +236,7 @@ func (e *Engine) computeSyncStats(ctx context.Context, syncID string) (*v3.SyncS } rec.SetGrants(grants) - assets, err := countKeyRange(ctx, e.db, AssetLowerBound(), AssetUpperBound(), nil) + assets, err := countKeyRange(ctx, db, AssetLowerBound(), AssetUpperBound(), nil) if err != nil { return nil, fmt.Errorf("computeSyncStats: assets: %w", err) } diff --git a/pkg/dotc1z/engine/pebble/test_seams.go b/pkg/dotc1z/engine/pebble/test_seams.go index 2bbe2382b..5441859fe 100644 --- a/pkg/dotc1z/engine/pebble/test_seams.go +++ b/pkg/dotc1z/engine/pebble/test_seams.go @@ -56,4 +56,14 @@ type testSeams struct { // stored record stays unstamped and the sync stays discoverable // as unfinished (resumable). endSyncStampHook func() error + + // currentSyncStepPreReadHook, when non-nil, runs inside + // CurrentSyncStep between sampling the binding generation and + // reading the sync-run record — the window the seqlock retry + // exists for. A hook that rebinds or clears the current sync moves + // the generation mid-read, which is the only way to reach the + // retry branch from a test: the transition has to land between two + // statements of another goroutine's read, and no amount of + // concurrent hammering can be made to guarantee it. + currentSyncStepPreReadHook func() } diff --git a/pkg/dotc1z/engine/pebble/write_barrier_owner.go b/pkg/dotc1z/engine/pebble/write_barrier_owner.go new file mode 100644 index 000000000..e6f2db47b --- /dev/null +++ b/pkg/dotc1z/engine/pebble/write_barrier_owner.go @@ -0,0 +1,127 @@ +package pebble + +import ( + "bytes" + "runtime" + "strconv" +) + +// Panic messages for the write barrier's ownership checks. Constants so +// the regression tests assert the exact value rather than a substring +// that could drift. The drain-side panics (waiting on your own write or +// pinned read) live with the admission gate in admission.go. +const ( + reentrantWriteBarrierPanic = "pebble engine: re-entrant write barrier — this goroutine already holds it, " + + "so an exported write was called from inside another write's body. Restructure the caller, or give " + + "the inner write an unexported sibling that runs under the barrier already held." + lifecycleFromWriteBarrierPanic = "pebble engine: sync-lifecycle transition called from inside a write body — " + + "the lock order is lifecycleMu then writeMu everywhere else, so taking lifecycleMu while holding the " + + "write barrier deadlocks against a concurrent EndSync. Hoist the transition out of the write." +) + +// writeBarrierOwnerChecks (lock_checks_enabled.go / _disabled.go) gates +// every deadlock-shape check at compile time: the barrier ownership +// checks below and the admission gate's self-wait checks (admission.go). +// A goroutine that waits on itself does so deterministically, on the +// first call, with no data dependence and no concurrency required — it +// cannot reach production without hanging the first armed run that +// exercises it, so the armed builds are `make test`, CI, and anything +// built with -race or -tags=baton_lockchecks. + +// lockWriteBarrier takes the engine's write barrier; unlockWriteBarrier +// releases it. +// +// writeMu is a plain, non-reentrant mutex, so calling an exported write +// from inside another write's body wedges that goroutine permanently: +// one goroutine, no output, no stack unless someone sends SIGQUIT. The +// mistake is easy to make because the composed call reads like every +// other write, and the engine offers no way to spell "these two writes +// go together" — so a contributor reaching for atomicity reaches for +// the exported method. This turns the hang into a panic that names what +// happened. +// +// A paired unlock rather than a returned release, which would read +// better: returning one is a heap allocation on every write (the method +// value in the unchecked branch escapes just as the closure does), and +// this check is supposed to cost production nothing. +func (e *Engine) lockWriteBarrier() { + if !writeBarrierOwnerChecks { + e.writeMu.Lock() + return + } + self := goroutineID() + // A self of 0 means the id was unreadable; 0 is also "unheld", so + // skip the comparison rather than report a barrier that is free as + // re-entered. + if self != 0 && e.writeBarrierOwner.Load() == self { + panic(reentrantWriteBarrierPanic) + } + e.writeMu.Lock() + e.writeBarrierOwner.Store(self) +} + +// unlockWriteBarrier releases the barrier taken by lockWriteBarrier. +func (e *Engine) unlockWriteBarrier() { + if writeBarrierOwnerChecks { + // Clear before unlocking, or the next holder inherits our id. + e.writeBarrierOwner.Store(0) + } + e.writeMu.Unlock() +} + +// trackedGoroutineID returns the calling goroutine's id, or 0 when the +// bookkeeping is off or the id was unreadable. Callers skip their +// bookkeeping on 0: it is also the "unheld" value, so recording it would +// make one unreadable id look like every other goroutine. +func trackedGoroutineID() uint64 { + if !writeBarrierOwnerChecks { + return 0 + } + return goroutineID() +} + +// assertNotTakingLifecycleFromWrite panics when the calling goroutine +// holds the write barrier. The sync-lifecycle transitions take +// lifecycleMu, and EndSync holds it across a finalize whose steps take +// the barrier — so a transition called from inside a write body supplies +// the writeMu → lifecycleMu order that deadlocks against it. +// +// EVERY transition calls this before taking lifecycleMu. The barrier +// re-entrancy check inside the transitions that write +// (startNewSync/CheckpointSync/EndSync) is not a substitute: it fires +// at the body's first inner write — after lifecycleMu is already held — +// and a contended lifecycleMu parks the caller before it ever gets +// there, holding the barrier a concurrent EndSync's finalize is waiting +// on. That is the deadlock, and only a check placed before the lock +// turns it into a panic. +func (e *Engine) assertNotTakingLifecycleFromWrite() { + if !writeBarrierOwnerChecks { + return + } + if self := goroutineID(); self != 0 && e.writeBarrierOwner.Load() == self { + panic(lifecycleFromWriteBarrierPanic) + } +} + +// goroutineID returns the calling goroutine's runtime id, or 0 if the +// runtime's format changes under us. Only reachable from the test-gated +// checks above: it formats a stack frame, which no production write path +// should pay for. +func goroutineID() uint64 { + var buf [64]byte + // "goroutine 123 [running]:" — the id is the second field. + line := buf[:runtime.Stack(buf[:], false)] + line, ok := bytes.CutPrefix(line, []byte("goroutine ")) + if !ok { + return 0 + } + i := bytes.IndexByte(line, ' ') + if i < 0 { + return 0 + } + id, err := strconv.ParseUint(string(line[:i]), 10, 64) + if err != nil { + return 0 + } + return id +} diff --git a/pkg/dotc1z/engine/pebble/write_barrier_reentry_test.go b/pkg/dotc1z/engine/pebble/write_barrier_reentry_test.go new file mode 100644 index 000000000..7a50d530d --- /dev/null +++ b/pkg/dotc1z/engine/pebble/write_barrier_reentry_test.go @@ -0,0 +1,233 @@ +// These tests assert the panics the deadlock-shape checks raise, so they +// only exist in builds where the checks are compiled in. Unarmed builds +// would hang exactly where these expect a panic. +//go:build baton_lockchecks || race + +package pebble + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" + + v3 "github.com/conductorone/baton-sdk/pb/c1/storage/v3" + "github.com/conductorone/baton-sdk/pkg/connectorstore" +) + +// stampSyncRunInsideWrite is the mistake the barrier check exists to +// catch, written the way someone would actually write it: a wrapper that +// wants a record write and a sync-run stamp to go together, so it calls +// the exported stamp from inside the write's body. Nothing at this call +// site says the barrier is already held. +func (e *Engine) stampSyncRunInsideWrite(ctx context.Context, syncID string) error { + return e.withWrite(func() error { + rec, err := e.GetSyncRunRecord(ctx, syncID) + if err != nil { + return err + } + return e.PutSyncRunRecord(ctx, rec) + }) +} + +// closeInsideWrite is the same mistake one step earlier in the teardown: +// Close waits for in-flight writes, and this goroutine's write is one of +// them, so it hangs on the wait rather than on the mutex. +func (e *Engine) closeInsideWrite() error { + return e.withWrite(func() error { + return e.Close() + }) +} + +// setCurrentSyncInsideWrite is the third shape of the same mistake, and +// the one that gets no help from the barrier: rebinding the current sync +// from inside a write body takes lifecycleMu while holding writeMu, the +// reverse of the order EndSync uses. Sequentially it works, which is what +// makes it dangerous — it needs a concurrent EndSync to deadlock, so it +// can pass review, pass tests, and hang in production. +func (e *Engine) setCurrentSyncInsideWrite(ctx context.Context, syncID string) error { + return e.withWrite(func() error { + return e.SetCurrentSync(ctx, syncID) + }) +} + +// TestLifecycleTransitionFromInsideWritePanics pins the guard on the two +// lifecycle transitions that never take the barrier. ResumeSync and +// SetCurrentSync read a record and rebind, touching currentSyncMu and +// sealMu but never writeMu, so neither the re-entrancy check nor the +// write drain sees them: without the explicit assertion this call +// returns cleanly and leaves the lock-order violation in the tree. +func TestLifecycleTransitionFromInsideWritePanics(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + syncID, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + require.PanicsWithValue(t, lifecycleFromWriteBarrierPanic, func() { + _ = e.setCurrentSyncInsideWrite(ctx, syncID) + }) + // Called normally, from outside a write, the same transition is fine. + require.NoError(t, e.SetCurrentSync(ctx, syncID)) + require.NoError(t, e.CheckpointSync(ctx, "after-panic")) +} + +// TestWriteBarrierPanicsOnReentry pins the check that turns a +// single-goroutine deadlock into a diagnosable failure. Without it the +// call below hangs forever with no output: writeMu is not reentrant, and +// there is no concurrency involved to make the hang look like a race. +func TestWriteBarrierPanicsOnReentry(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + syncID, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + require.PanicsWithValue(t, reentrantWriteBarrierPanic, func() { + _ = e.stampSyncRunInsideWrite(ctx, syncID) + }) + + // The panic unwinds through the outer write's release, so the barrier + // is free and the engine still works. A check that left the barrier + // held would trade one hang for another. + require.NoError(t, e.CheckpointSync(ctx, "after-panic")) + step, err := e.CurrentSyncStep(ctx) + require.NoError(t, err) + require.Equal(t, "after-panic", step) +} + +// TestCloseFromInsideWritePanics covers the wait-side variant: Close and +// CheckpointTo drain admitted writes before they reach the barrier, so the +// ownership comparison has to happen there too or these hang one step +// short of the mutex the re-entrancy check watches. +func TestCloseFromInsideWritePanics(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + _, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + require.PanicsWithValue(t, writeBarrierWaitFromWritePanic, func() { + _ = e.closeInsideWrite() + }) + require.NoError(t, e.CheckpointSync(ctx, "still-open")) +} + +// TestWriteBarrierAdmitsSequentialAndConcurrentWrites is the negative +// control: the check must fire on one goroutine holding the barrier +// twice, and never on the ordinary cases. Writers contending for the +// barrier hand ownership back and forth, which is exactly where a +// bookkeeping bug (clearing after the unlock, say) would report the next +// holder as a re-entrant one. +// +// The concurrent phase writes grants rather than checkpoints, and that is +// the difference between contending and looking like it: CheckpointSync +// holds lifecycleMu across its whole body and only reaches the barrier +// inside PutSyncRunRecord, so concurrent checkpoints queue on the +// lifecycle mutex and reach writeMu one at a time. PutGrants takes the +// barrier with no lifecycle lock above it. +func TestWriteBarrierAdmitsSequentialAndConcurrentWrites(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + _, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + for i := 0; i < 4; i++ { + require.NoError(t, e.CheckpointSync(ctx, "sequential")) + } + + const writers = 8 + errs := make(chan error, writers) + for i := 0; i < writers; i++ { + go func(i int) { + errs <- e.PutGrants(ctx, mkV2Grant( + fmt.Sprintf("grant-%d", i), + fmt.Sprintf("entitlement-%d", i), + "user", + fmt.Sprintf("principal-%d", i), + )) + }(i) + } + for i := 0; i < writers; i++ { + require.NoError(t, <-errs) + } +} + +// closeInsidePinnedRead is the read-side shape of the same mistake, and +// the one the engine invites: Iterate* holds the pin across the yield +// callback, so a caller that decides mid-scan it is done and closes the +// engine is waiting for the read it is still inside. +func (e *Engine) closeInsidePinnedRead(ctx context.Context) error { + return e.IterateGrants(ctx, func(*v3.GrantRecord) bool { + _ = e.Close() + return false + }) +} + +// TestCloseFromInsidePinnedReadPanics covers the read half of the drain. +// Close drains reads as well as writes, so the wait-side check has to +// see pinned readers too — a scan callback is caller-supplied code +// running with the pin held, which makes this the easiest way to reach +// the hang from outside the package. +func TestCloseFromInsidePinnedReadPanics(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + _, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, e.PutGrants(ctx, mkV2Grant("grant-1", "entitlement-1", "user", "principal-1"))) + + require.PanicsWithValue(t, readPinWaitFromReadPanic, func() { + _ = e.closeInsidePinnedRead(ctx) + }) + // The panic unwinds through the scan's release, and nothing was + // marked closing, so the engine is still usable. + require.NoError(t, e.CheckpointSync(ctx, "after-panic")) +} + +// TestCloseFromInsideWriteRacingAnotherClosePanics pins where the +// assertion sits relative to closeMu. One Close already holds the lock +// and is parked draining writes; the goroutine holding the write it waits +// for is the one calling here. Behind the lock this caller would block on +// closeMu and never reach the check — the same silent hang the check +// exists to report, moved one lock earlier. +func TestCloseFromInsideWriteRacingAnotherClosePanics(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + _, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + require.NoError(t, e.admit.enterWrite()) + closed := make(chan error, 1) + go func() { closed <- e.Close() }() + // The flag flips under closeMu on the way to the drain, so observing + // it means the other Close holds the lock this caller would queue on. + require.Eventually(t, func() bool { return e.admit.isClosing() }, 10*time.Second, time.Millisecond, + "the concurrent Close never reached its drain") + + require.PanicsWithValue(t, writeBarrierWaitFromWritePanic, func() { + _ = e.Close() + }) + e.admit.exitWrite() + require.NoError(t, <-closed) +} + +// TestCloseFromBarrierFreeAdmittedWritePanics covers the gate-admitted +// writes that never take the barrier. CompactAllRanges and Flush enter +// the gate for the length of a compaction or flush deliberately without +// writeMu, and cleanup.go documents that a Close during either one hangs +// on the drain — so a check keyed on barrier ownership would see nothing +// wrong with the very hang it is there to report. Neither has an +// injection point mid-operation, so this enters the gate the same way +// they do. +func TestCloseFromBarrierFreeAdmittedWritePanics(t *testing.T) { + ctx := context.Background() + e, _ := newTestEngine(t) + _, err := e.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + require.NoError(t, e.admit.enterWrite()) + defer e.admit.exitWrite() + require.Zero(t, e.writeBarrierOwner.Load(), "a gate-admitted write that skipped the barrier must not own it") + require.PanicsWithValue(t, writeBarrierWaitFromWritePanic, func() { + _ = e.Close() + }) +} diff --git a/pkg/sync/external_principal_index.go b/pkg/sync/external_principal_index.go index 06792e02f..22f4de780 100644 --- a/pkg/sync/external_principal_index.go +++ b/pkg/sync/external_principal_index.go @@ -190,7 +190,7 @@ func foldKey(s string) string { var b strings.Builder b.Grow(len(s)) for _, r := range s { - b.WriteRune(foldRune(r)) + _, _ = b.WriteRune(foldRune(r)) } return b.String() }