Repository navigation
build(deps): fix SQLite lock-wait cancellation - #169
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex review: needs changes before merge. What this changesUpgrade SQLite and libc to make database lock waits honor context deadlines, with a regression test and documentation updates. Example: A write has a 100 ms deadline while another transaction holds the database lock.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness⛔ Needs changes before merge - 1 item remains Keep this PR open and paused: cancellation remains unfixed on main, while the proposed dependency upgrade breaks the pinned downstream JSON assertion. Priority: P2 Before merge
Findings
Agent review detailsHow this fits togetherCrawlkit’s store package opens SQLite connections for downstream archive applications; caller SQL and contexts enter through database/sql, and query results or cancellation errors return through the driver. flowchart TD
A[Downstream archive application] --> B[Crawlkit store]
B --> C[Go database sql]
C --> D[Modernc SQLite driver]
D --> E[SQLite archive file]
D --> F[Results and context errors]
Technical reviewBest possible solution: Keep the published runtime pair until a cancellation fix preserves downstream behavior or a separately validated downstream correction resolves the JSON assertion. Do we have a high-confidence way to reproduce the issue? The temporary-file regression provides a concrete lock-contention reproducer, and the body records failure with the previous driver and success with the candidate; this review did not execute it. Is this the best way to solve the issue? Updating the shared driver is appropriately scoped, but this version is unsuitable while its independently identified downstream assertion fails. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 4777448a4b69. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 23, 2026, 3:28 AM ET / 07:28 UTC. |
The cloud snapshot privacy check relied on SQLite preserving a JSON subtype through a scalar subquery. SQLite 3.54.1 quotes that nested array, so the check failed despite unchanged sanitization. Explicitly mark the subquery result with `json(...)` and preserve the exact expected array, including nulls and cleared private strings. This unblocks the SQLite lock-cancellation update in openclaw/crawlkit#169 without weakening its pinned downstream compatibility gate. No production query or sanitizer behavior changes. Validation: reproduced the existing failure with modernc.org/sqlite v1.61.0 (SQLite 3.54.1); all `TestCloudSQLiteSnapshot*` tests pass with both v1.60.1 (SQLite 3.53.4) and v1.61.0 using isolated module files. Independent Codex autoreview is clean through P3. CI runs the full repository gates.
Prepare Crawlkit 0.17.3, a patch release for the SQLite write-lock cancellation fix from #169. The release preserves the Go 1.27.0 minimum, uses the matching SQLite v1.61.0/libc v1.79.0 pair, and keeps the migrated Gitcrawl compatibility gate. Move the existing Unreleased bullet into a dated release section with a one-line highlight. Every existing changelog bullet is preserved, and the empty Unreleased header remains for future work. Validation: #169's exact-head and post-merge CI are green, including the full downstream comparison. Full make check, govulncheck, and race tests passed during the fix; actionlint passes. This PR changes only the changelog. Independent Codex autoreview is clean through P3; CI repeats the complete gates on this head. Release publication follows the signed-tag GitHub Actions process after the merged-head dry run.
A write blocked by another transaction took 5.10 seconds to return with a 100 ms context deadline. Upgrade modernc.org/sqlite to v1.61.0 and its required libc v1.79.0 runtime so busy waits honor cancellation, preserving the Go 1.27.0 minimum and Go 1.27.3 build toolchain.
The regression through
store.Openchecks the deadline error, bounded return time, no unintended insert, and a successful retry after releasing the lock. Dependency-pair and explicit JSON-subquery documentation are updated.Downstream migration completed
Gitcrawl #271 is merged. Its synthetic snapshot privacy assertion now wraps the scalar JSON aggregate in
json(...), preserving exactly[[null,"",""],"",""]on SQLite 3.53.4 and 3.54.1. Production sanitization is unchanged. The earlier compatibility hold is resolved.The downstream consumer audit found only that Gitcrawl test query depended on implicit subtype propagation. Notcrawl uses stored scalar fields; Photoscrawl directly decodes its album aggregate on read. Synthetic checks preserve those results on both SQLite versions. The other seven consumers have no SQL JSON constructors requiring migration.
Advance the pinned Gitcrawl source to its merged correction, keeping the existing published Crawlkit baseline, both complete CLI suites, timeouts, and isolation/integrity checks intact.
Validation
make checkpasses on AWS Crabbox: tidy, formatting, vet, deadcode, govulncheck, unit/race tests, and all 48 script tests.actionlintand all 48 script tests pass after the pin/documentation update.No downstream release is included. After publishing Crawlkit v0.17.3, downstream consumers can bump and release separately.