Default to exclusive locks on c1zs. #614
Conversation
WalkthroughC1File init now checks for existing locking pragmas (case-insensitive) and sets PRAGMA main.locking_mode = EXCLUSIVE when none found. Tests and applied DBs explicitly set locking_mode = NORMAL. Compactor no longer applies certain read-only pragmas and now logs Close() errors. Changes
Sequence Diagram(s)(Skipped — changes are limited to initialization defaults, test adjustments, and pruning pragmas; no new multi-component control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/synccompactor/attached/attached_test.go (2)
42-52: Consider verifying additional test data.The function currently only verifies user resources, while
putTestDataalso creates groups, entitlements, and grants. Consider adding verification for these additional entities to ensure complete data integrity through compaction.Example: Verify groups, entitlements, and grants
// After verifying users, add: groupsResp, err := db.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ ResourceTypeId: "group", }.Build()) require.NoError(t, err) require.Equal(t, 1, len(groupsResp.GetList())) grantsResp, err := db.ListGrants(ctx, v2.GrantsServiceListGrantsRequest_builder{}.Build()) require.NoError(t, err) require.Equal(t, 2, len(grantsResp.GetList()))
190-191: Consider using distinct data for multiple syncs.The test calls
putTestDatatwice (once for each sync), inserting identical data both times. If the intent is to verify that multiple syncs are handled correctly, consider populating distinct data in each sync (e.g., user3 and user4 in the second sync) to better validate the multi-sync compaction scenario.If the current approach is intentional to test idempotency, consider adding a comment explaining this.
Also applies to: 200-201
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/dotc1z/c1file.gopkg/dotc1z/session_store_test.gopkg/synccompactor/attached/attached_test.gopkg/synccompactor/attached/comprehensive_test.gopkg/synccompactor/attached/timestamp_test.gopkg/synccompactor/compactor.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 0
File: :0-0
Timestamp: 2025-12-31T21:17:25.009Z
Learning: In pkg/dotc1z/c1file.go, the `journal_mode = OFF` pragma is executed before applying per-pragma settings when `readOnly` is true, so that it can be overridden by user configuration if needed.
📚 Learning: 2025-12-31T21:17:25.009Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 0
File: :0-0
Timestamp: 2025-12-31T21:17:25.009Z
Learning: In pkg/dotc1z/c1file.go, the `journal_mode = OFF` pragma is executed before applying per-pragma settings when `readOnly` is true, so that it can be overridden by user configuration if needed.
Applied to files:
pkg/dotc1z/session_store_test.gopkg/synccompactor/compactor.gopkg/synccompactor/attached/timestamp_test.gopkg/dotc1z/c1file.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/synccompactor/attached/comprehensive_test.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/synccompactor/attached/comprehensive_test.go
🧬 Code graph analysis (4)
pkg/dotc1z/session_store_test.go (1)
pkg/dotc1z/c1file.go (2)
NewC1ZFile(180-216)WithPragma(151-155)
pkg/synccompactor/attached/comprehensive_test.go (1)
pkg/dotc1z/c1file.go (2)
WithPragma(151-155)NewC1ZFile(180-216)
pkg/synccompactor/attached/attached_test.go (3)
pkg/dotc1z/c1file.go (3)
C1File(37-60)WithPragma(151-155)NewC1ZFile(180-216)pkg/connectorstore/connectorstore.go (4)
SyncTypePartial(15-15)SyncType(11-11)SyncTypeAny(17-17)SyncTypeFull(14-14)pkg/synccompactor/attached/attached.go (1)
NewAttachedCompactor(19-24)
pkg/synccompactor/attached/timestamp_test.go (1)
pkg/dotc1z/c1file.go (2)
WithPragma(151-155)NewC1ZFile(180-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (12)
pkg/dotc1z/session_store_test.go (1)
1693-1693: LGTM! Necessary configuration for concurrent access testing.The addition of
WithPragma("main.locking_mode", "NORMAL")is essential for this test, which spawns multiple goroutines performing concurrent reads and writes. With the new default EXCLUSIVE locking mode introduced in c1file.go, NORMAL mode must be explicitly set to allow the concurrent access patterns this test validates.pkg/synccompactor/compactor.go (2)
164-169: Good improvement: logging Close() errors.Previously, Close() errors were silently ignored. Now they are properly logged with context about the compacted file path. This improves observability and debugging.
297-298: The concern in this review comment is not warranted.WithReadOnly(true)is application-level enforcement only (preventing database writes on close), not SQLite-level read-only mode, since the go-sqlite library does not support true read-only mode. ThePRAGMA main.locking_mode = EXCLUSIVEis applied as a normal pragma after initialization and works without issues. The database file can be attached successfully viaAttachFilewithout problems, as confirmed by existing test coverage.pkg/dotc1z/c1file.go (1)
284-297: LGTM! Clean implementation of default EXCLUSIVE locking.The logic correctly:
- Scans existing pragmas with case-insensitive comparison for "main.locking_mode" or "locking_mode"
- Applies
PRAGMA main.locking_mode = EXCLUSIVEas a default only if no locking pragma exists- Executes before user-specified pragmas (lines 299-304), allowing users to override the default
This ordering ensures read-only databases get
journal_mode = OFFandsynchronous = OFFfirst (lines 271-282), then the locking default, then user overrides—maintaining the flexibility described in the learnings.Based on learnings, read-only pragmas are applied before per-pragma settings to allow user override.
pkg/synccompactor/attached/comprehensive_test.go (2)
22-22: Good practice: using test context.Switching from
context.Background()tot.Context()ensures proper test lifecycle management and cancellation propagation.
125-126: LGTM! Necessary locking configuration for compaction.Creating
appliedOptswithlocking_mode = normalis essential for the compactor to function correctly. During compaction, the base database needs to attach and read from the applied database concurrently, which requires NORMAL locking mode on the applied database (rather than the default EXCLUSIVE mode). The use ofslices.Cloneproperly avoids mutating the originaloptsslice.pkg/synccompactor/attached/timestamp_test.go (2)
19-19: Good practice: using test context.Switching to
t.Context()enables proper test cancellation and lifecycle management.
62-63: LGTM! Consistent locking configuration across test scenarios.Both test scenarios correctly configure
appliedOptswithlocking_mode = normalto enable the compactor to attach and read from the applied database. This is consistent with the pattern in comprehensive_test.go and necessary for compaction to function with the new default EXCLUSIVE locking mode.pkg/synccompactor/attached/attached_test.go (4)
6-6: LGTM! Imports support new test helpers.The new imports are correctly used throughout the file for cloning options, building test data structures, and querying sync metadata.
Also applies to: 9-10
16-40: LGTM! Well-structured test data helper.The function consistently populates a complete data model hierarchy (resource types → resources → entitlements → grants) with proper error handling.
55-55: LGTM! Better test lifecycle management.Using
t.Context()instead ofcontext.Background()ensures proper cleanup when tests complete or are cancelled.Also applies to: 108-108, 156-156
88-88: LGTM! Comprehensive test data flow verification.The addition of
putTestDataandverifyTestDatacalls ensures that actual data flows through compaction correctly. The sync metadata verification (lines 97-104) is particularly valuable for confirming that sync type is preserved after compaction.Also applies to: 97-104
| appliedOpts := append(slices.Clone(opts), dotc1z.WithPragma("locking_mode", "normal")) | ||
| appliedDB, err := dotc1z.NewC1ZFile(ctx, appliedFile, appliedOpts...) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for NewAttachedCompactor usage
echo "=== NewAttachedCompactor usage in production code ==="
rg -n 'NewAttachedCompactor' --type go -g '!*test*'
echo -e "\n=== All NewAttachedCompactor calls (including tests) ==="
rg -n 'NewAttachedCompactor' --type go -B2 -A5
echo -e "\n=== NewC1ZFile calls in production (non-test) code ==="
rg -n 'NewC1ZFile' --type go -g '!*test*' -B2 -A2
echo -e "\n=== Search for appliedOpts or similar pattern in production ==="
rg -n 'appliedDB.*NewC1ZFile|appliedOpts' --type go -g '!*test*' -B3 -A3Repository: ConductorOne/baton-sdk
Length of output: 8036
🏁 Script executed:
#!/bin/bash
echo "=== compactor.go lines 150-170 (where compactedC1z is created) ==="
sed -n '150,170p' pkg/synccompactor/compactor.go
echo -e "\n=== compactor.go lines 280-320 (where applyFile is created and compactor used) ==="
sed -n '280,320p' pkg/synccompactor/compactor.go
echo -e "\n=== Check how opts is defined in doOneCompaction ==="
rg -n 'opts\s*:=|opts\s*=' pkg/synccompactor/compactor.go -B2 -A2 | head -40Repository: ConductorOne/baton-sdk
Length of output: 2193
🏁 Script executed:
#!/bin/bash
echo "=== Search for WithReadOnly in c1file.go ==="
rg -n 'WithReadOnly|readOnly' pkg/dotc1z/c1file.go -B3 -A3
echo -e "\n=== Check c1file.go around journal_mode and locking_mode pragmas ==="
rg -n 'journal_mode|locking_mode' pkg/dotc1z/c1file.go -B2 -A2Repository: ConductorOne/baton-sdk
Length of output: 2329
🏁 Script executed:
#!/bin/bash
echo "=== attached.go Compact method and ATTACH usage ==="
cat -n pkg/synccompactor/attached/attached.go | head -100Repository: ConductorOne/baton-sdk
Length of output: 3823
🏁 Script executed:
#!/bin/bash
echo "=== Search for comments around locking or ATTACH in compactor.go ==="
sed -n '280,320p' pkg/synccompactor/compactor.go | head -50
echo -e "\n=== Check if there's any issue or TODO about locking_mode ==="
rg -n -i 'locking|attach.*locking|exclusive.*normal' pkg/synccompactor/ --type go -B2 -A2Repository: ConductorOne/baton-sdk
Length of output: 4644
Production code should either apply the same locking_mode override to applied databases as tests do, or document why it's not needed.
The tests consistently override the applied database to use locking_mode=normal (lines 79-80, 133-134, 180-181, and in other test files), while the base database uses the default EXCLUSIVE mode (set at compactor.go:147). However, the production code in compactor.go:291-298 creates the applied database without this override, meaning it defaults to EXCLUSIVE locking mode just like the base database.
When NewAttachedCompactor performs the SQLite ATTACH operation (attached.go:50), both databases would have EXCLUSIVE locking in production but EXCLUSIVE and NORMAL in tests. This mismatch suggests either:
- The override is necessary for correct behavior during ATTACH operations and should be applied in production, or
- The applied database being
ReadOnlymakes the override unnecessary in practice, and tests are being overly cautious
Clarify the intent and ensure consistency between test and production behavior.
ggreer
force-pushed
the
ggreer/exclusive-lock
branch
from
e849ef8 to
1e559ee
Compare
Except for compaction, nothing should be writing to the same c1 file at the same time. Also improve some of the compaction tests. Now they insert data and validate the results.
ggreer
force-pushed
the
ggreer/exclusive-lock
branch
from
1e559ee to
bd54d6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/synccompactor/attached/comprehensive_test.go (1)
125-126: LGTM!The applied database correctly uses NORMAL locking mode to allow attachment during compaction. The use of
slices.Clone(opts)prevents mutation of the original options, and appending thelocking_mode="normal"pragma ensures the applied database can be attached to the base database without locking conflicts.This aligns with the PR's objective to default to exclusive locks while allowing normal mode where needed for compaction operations.
Optional: Consider adding a clarifying comment
While the implementation is correct, you could optionally add a brief comment explaining why the applied database requires normal locking mode:
// ========= Create applied database ========= +// Use normal locking mode to allow attachment during compaction appliedOpts := append(slices.Clone(opts), dotc1z.WithPragma("locking_mode", "normal")) appliedDB, err := dotc1z.NewC1ZFile(ctx, appliedFile, appliedOpts...)This is a minor documentation enhancement and not strictly necessary.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/dotc1z/c1file.gopkg/dotc1z/session_store_test.gopkg/synccompactor/attached/attached_test.gopkg/synccompactor/attached/comprehensive_test.gopkg/synccompactor/attached/timestamp_test.gopkg/synccompactor/compactor.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/dotc1z/c1file.go
- pkg/synccompactor/attached/attached_test.go
- pkg/synccompactor/attached/timestamp_test.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 0
File: :0-0
Timestamp: 2025-12-31T21:17:25.024Z
Learning: In pkg/dotc1z/c1file.go, the `journal_mode = OFF` pragma is executed before applying per-pragma settings when `readOnly` is true, so that it can be overridden by user configuration if needed.
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/synccompactor/attached/comprehensive_test.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/synccompactor/attached/comprehensive_test.go
📚 Learning: 2026-01-02T17:21:01.723Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.
Applied to files:
pkg/synccompactor/attached/comprehensive_test.gopkg/dotc1z/session_store_test.go
📚 Learning: 2025-12-31T21:17:25.024Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 0
File: :0-0
Timestamp: 2025-12-31T21:17:25.024Z
Learning: In pkg/dotc1z/c1file.go, the `journal_mode = OFF` pragma is executed before applying per-pragma settings when `readOnly` is true, so that it can be overridden by user configuration if needed.
Applied to files:
pkg/dotc1z/session_store_test.gopkg/synccompactor/compactor.go
🧬 Code graph analysis (2)
pkg/synccompactor/attached/comprehensive_test.go (1)
pkg/dotc1z/c1file.go (2)
WithPragma(152-156)NewC1ZFile(181-217)
pkg/dotc1z/session_store_test.go (1)
pkg/dotc1z/c1file.go (2)
NewC1ZFile(181-217)WithPragma(152-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, windows-latest)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
🔇 Additional comments (5)
pkg/dotc1z/session_store_test.go (1)
1693-1693: LGTM! Correct adjustment for concurrent access testing.The addition of
WithPragma("main.locking_mode", "NORMAL")is necessary and appropriate. Since the PR changes the default to EXCLUSIVE locking mode, this test—which specifically validates concurrent reads and writes with multiple goroutines—requires NORMAL mode to function correctly. The change is minimal and targeted to only the test that needs concurrent access.pkg/synccompactor/compactor.go (2)
162-167: Good observability improvement.Logging Close() errors that were previously ignored helps with debugging and ensures cleanup issues are visible.
289-295: Pragma removal aligns with PR objectives.The removal of write-related pragmas (synchronous, journal_mode) is appropriate since the file is opened read-only. Removing locking_mode EXCLUSIVE aligns with the PR goal to use default locking behavior during compaction, while exclusive locks are applied elsewhere by default.
pkg/synccompactor/attached/comprehensive_test.go (2)
5-5: LGTM!The
slicesimport is correctly added to supportslices.Clone(opts)on line 125.
22-22: LGTM!Using
t.Context()is the correct practice for test context management, providing proper lifecycle handling and cancellation support.
Except for compaction, nothing should be writing to the same c1 file at the same time.
Also improve some of the compaction tests. Now they insert data and validate the results.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.