Add the portable ClientSQL runtime and debugger - #177
Conversation
📊 PR Size: size/XLTotal changes: 7106 lines (51 files) Top files changed:
...and 41 more files Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+) |
github-actions
Bot
added
area/compiler
Sensitive Files Detected📦 Dependency change — Modifies the Bazel module graph — needs runtime team review after import. 📎 Prebuilt binary — Changes a prebuilt binary — requires verification of provenance. 🔧 Build rules — Affects build rules for all Valdi consumers. This is an automated notice. A maintainer will review after import. |
|
| Test Suite | Result |
|---|---|
| Snapshot Tests | ✅ success |
| Test Coverage Delta | ✅ success |
| Linux: Hotreload Smoke | ✅ success |
| Linux: Module Tests | ✅ success |
| API Surface Check | ✅ success |
| Valdi Smoke Tests | ❌ failure |
| Linux: Build Compiler | ✅ success |
| Linux: Build & Export | ✅ success |
| macOS: C++ & Platform Tests | ❌ failure |
| Linux: Registry Validation | ✅ success |
| valdi_web Integration Test | ✅ success |
| Linux: C++ Tests | ❌ failure |
Some tests failed. Please check the workflow logs for details.
🚀 Bazel remote cache is now enabled - future builds will be faster!
Workflow: Valdi CI
clholgat
left a comment
clholgat
left a comment
There was a problem hiding this comment.
Focused review of the ClientSQL runtime — native bridge, path handling, and debugger provider. The parameterized query paths, the path-traversal defenses in resolveDatabasePath, and the bounded read-only debugger provider all look solid. A few substantive items are inline below.
Note: this feedback should carry into the squashed PRs once the 24-PR stack is collapsed to ~3.
| @@ -0,0 +1,112 @@ | |||
| /** | |||
| * @ExportModule | |||
| * @Version(__PLACEHOLDER__) | |||
There was a problem hiding this comment.
🔴 High — Unresolved @Version(__PLACEHOLDER__). This exported native declaration (plus the other placeholder annotations in this file at L25/L34/L51 and the two in the client_sql_smoke testdata module) ships an invalid, non-numeric API version. As written this can't merge on the public repo — it hard-couples to an internal-only version-finalization step. Assign the concrete runtime API version to all six annotations (and the matching generator-test expectation) before merge.
| else len(native_contract) | ||
| ) | ||
| declaration_block = native_contract[declaration.start():next_start] | ||
| self.assertEqual(1, declaration_block.count("@Version(__PLACEHOLDER__)")) |
There was a problem hiding this comment.
🔴 High — This generator-test expectation still asserts @Version(__PLACEHOLDER__) — the seventh placeholder occurrence that must be finalized together with the six exported annotations. Update it to the concrete version so the test locks in the finalized value instead of the placeholder.
| _readerConnections.reserve(kClientSQLReaderConnectionCount); | ||
| for (size_t index = 0; index < kClientSQLReaderConnectionCount; ++index) { | ||
| if (fallbackQueue != nullptr) { | ||
| _readerConnections.emplace_back(makeShared<ClientSQLQueueConnection>(fallbackQueue)); |
There was a problem hiding this comment.
🟠 Med — The writer and all four reader connections are constructed from the same fallbackQueue (source is runtime->getWorkerQueue(), L1637); no per-reader queue is ever created. Pick one: (a) if the queue is serial, the READONLY+WAL reader pool delivers zero read/write concurrency (dead complexity); (b) if it's ever made concurrent to gain that concurrency, the non-atomic writer state (_activeTransactionId, _nextTransactionId, _deferredWriterWork, _schemaFingerprint) races. Give readers their own queue(s), or drop the pool and document the single-queue serialization.
| Path databaseRoot = storageRoot.appending(kClientSQLDirectory); | ||
| databaseRoot.normalize(); | ||
|
|
||
| Path databasePath = databaseRoot.appending(name.toStringView()); |
There was a problem hiding this comment.
🟠 Med — The path is derived from diskCache root + "ClientSQLNative" + name with no module/bundle component, though the header documents name as a "module-scoped storage identity". Cross-module isolation then rests entirely on getDiskCache() returning a per-module root. If it returns the shared runtime cache, two modules that pick the same generated package name collide: the schema-fingerprint guard denies the second opener (DoS), or with a matching schema they silently share one file (data leak). Please confirm getDiskCache() is per-module, or add an explicit module/bundle segment to the path.
| const ClientSQLOpenRequest& request); | ||
|
|
||
| Result<Void> executeStatement(sqlite3* database, const ClientSQLRequest& request) { | ||
| auto statement = prepareStatement(database, request.sql); |
There was a problem hiding this comment.
🟢 Low — executeStatement/queryStatement prepare via sqlite3_prepare_v2, which compiles only the first statement of the text (schema/migration paths correctly use sqlite3_exec). A raw multi-statement execute(sql, ...) therefore silently runs only the first statement and drops the rest. Consider looping over the tail, or rejecting trailing SQL, so multi-statement input isn't silently ignored.
2 participants
Description
Adds the complete portable ClientSQL runtime, native and web implementations, SQLite toolchain validation, generated integration, and debugger provider as one coherent native feature.
Type of Change
Testing
bazel test //...)Testing Details
//src/valdi_modules/src/valdi/client_sql:client_sql_native_testspassed.//valdi:test_client_sql_runtime_integrationpassed.bazel query //...completed successfully with 9,688 targets.Native API finalization
This PR introduces a new exported native API and intentionally carries six
@Version(__PLACEHOLDER__)annotations. During Snap's internal import/finalizationprocess, these declarations must be assigned one concrete runtime API version.
There are seven PR-added placeholder occurrences to finalize together: the six
annotations and the corresponding generator-test expectation. Existing
placeholder infrastructure and fixtures elsewhere in the repository must remain
unchanged.
The public Valdi repository does not currently expose a Native API Finalization
workflow, so this PR must not be merged with these declarations unresolved.
Please coordinate the version allocation with the internal importer/maintainer.
Checklist
Related Issues
Relates to #154
Additional Context
Optional ClientSQL side stack, stacked on generator PR #176 (
bjd/client-sql-generator). This stack is not required by the core debugger chain. Review this PR as the single incremental commit428e1ace; do not merge it before its parent or before native API finalization.