Repository navigation
fix(server): return SQLite write versions atomically - #4408
Merged
johntmyers merged 2 commits intoOct 12, 2026
Merged
johntmyers merged 2 commits into
johntmyers merged 2 commits into
Conversation
Use RETURNING instead of a follow-up read. Signed-off-by: Eric Curtin <eric.curtin@docker.com>
ericcurtin
requested review from
a team,
derekwaynecarr,
mrunalp and
sjenning
as code owners
October 11, 2026 20:33
Contributor
Author
|
@johntmyers @elezar PTAL when you get a chance, and |
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
Collaborator
|
/ok to test 73eddf6 |
|
Label |
johntmyers
reviewed
Oct 12, 2026
johntmyers
left a comment
johntmyers
left a comment
Collaborator
There was a problem hiding this comment.
gator-agent
PR Review Status
@ericcurtin, I reviewed the latest head in response to your review and testing request. Both SQLite write paths now return their own version and timestamps atomically, and the concurrent-writer regression test covers both paths. No blocking findings remain.
I applied test:e2e and authorized the current-head test mirror. Branch Checks, Helm Lint, and E2E are running; Trivy Changes is queued after workflow approval. Gator will inspect the results next cycle.
Blocking findings: None.
Carried findings: None.
Non-blocking suggestions: None.
Gator metadata
- Validation: Focused persistence correctness fix for the reproducible race in #4401; no duplicate implementation found.
- Docs: Not needed; restores existing write-result semantics without adding a direct UX or configuration change.
- Checks: DCO passed; Branch Checks and Helm Lint running; Trivy Changes queued on current head.
- E2E:
test:e2eapplied; current-head Branch E2E Checks running with build jobs active. - Head SHA:
73eddf6ca34af086b7326616e351420699dda2b4 - Base SHA:
eeba0e7954c0fb4d8e9e2e29d1bfa68e290eb8b3 - Merge base SHA:
eeba0e7954c0fb4d8e9e2e29d1bfa68e290eb8b3 - Patch ID:
144e9bdf8cbee1027a37d30c4aaac120db127e31 - Gator payload:
12 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
johntmyers
approved these changes
Oct 12, 2026
Collaborator
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: Gator found no blocking review findings. Branch Checks, Helm Lint, Trivy Changes, and E2E passed, and a maintainer approval was submitted before merge. I removed the active Gator metadata
|
johntmyers
removed
the
gator:approval-needed
4 tasks
krishicks
added a commit
that referenced
this pull request
Oct 23, 2026
sqlite_concurrent_writes_return_their_own_resource_version (#4408) runs 8 writers that each make 25 unconditional and 25 version-checked writes against one row of an on-disk WAL database with synchronous=FULL. Writers serialize on the SQLite write lock and wait out sqlx's default 5s busy_timeout. Prior to this change, the per-commit fsync cost on the Windows runners was high enough that a waiting writer starved past that timeout and failed with "database is locked" after about 7s. Now, each writer makes 5 writes on Windows and 25 elsewhere. The test still races every writer on the same row, which is what it checks. Signed-off-by: Kris Hicks <khicks@nvidia.com>
purp
pushed a commit
to purp/OpenShell
that referenced
this pull request
Oct 23, 2026
* fix(isolation): validate Unix socket paths the same way on every host Prior to this change, SandboxTransport::validate checked Unix socket paths with Path::is_absolute(). On Windows that requires a drive or UNC prefix, so a POSIX path such as /run/backend.sock was rejected as "invalid boundary control transport endpoint". The isolation backend registration work in NVIDIA#3898 added tests that register Unix endpoints in the core registration types, gateway config, gateway config preflight, and supervisor backend setup; all of them failed on the Windows MSVC runners. The Windows test job only runs on PRs labeled test:windows, so the break went unnoticed on main. Unix transport is refused at runtime on non-Unix hosts, so these paths are always POSIX paths regardless of where they are validated. Now, both the core registration validator and the sandbox backend protocol validator use a new openshell_core::paths::is_posix_absolute helper, which applies the POSIX rule (the path starts with '/') on every host. On Unix this matches is_absolute() exactly. Signed-off-by: Kris Hicks <khicks@nvidia.com> * test(supervisor): run baseline write-back cases only on Unix hosts The startup policy write-refusal tests from NVIDIA#3785 rely on enrich_proto_baseline_paths adding the proxy baseline paths (/usr, /tmp, /dev/null, ...) to a policy, which triggers a write-back that the fake gateway then refuses. Enrichment only adds paths that exist on the host. Prior to this change, the tests assumed those paths exist, so on Windows enrichment added nothing, the precondition assertions failed, and the baseline case of permanent_codes_still_exit never saw a write to refuse. Now, the four baseline write-back tests are gated to Unix, and permanent_codes_still_exit checks the image policy upload case on every host and the baseline write-back case only on Unix. Signed-off-by: Kris Hicks <khicks@nvidia.com> * test(server): scale the SQLite concurrent-write test for Windows fsync sqlite_concurrent_writes_return_their_own_resource_version (NVIDIA#4408) runs 8 writers that each make 25 unconditional and 25 version-checked writes against one row of an on-disk WAL database with synchronous=FULL. Writers serialize on the SQLite write lock and wait out sqlx's default 5s busy_timeout. Prior to this change, the per-commit fsync cost on the Windows runners was high enough that a waiting writer starved past that timeout and failed with "database is locked" after about 7s. Now, each writer makes 5 writes on Windows and 25 elsewhere. The test still races every writer on the same row, which is what it checks. Signed-off-by: Kris Hicks <khicks@nvidia.com> * test(server): hold scope-mix locks for wall-clock time, not timer ticks random_scope_mix_never_deadlocks (NVIDIA#4147) runs 64 tasks of 50 random lock operations and holds each operation's locks for 0-2 ms, failing if the mix does not finish within 20s. Prior to this change, the hold was a tokio::time::sleep. Windows timers round a 1 ms sleep up to about 16 ms, and a quarter of the operations take the exclusive global lock, so the serialized holds alone took several seconds and the full mix exceeded the 20s deadline on the Windows runners. Now, occupy yields until a std::time::Instant deadline, so holds last 0-2 ms of wall-clock time on every platform and the deadline means the same thing everywhere. Signed-off-by: Kris Hicks <khicks@nvidia.com> * test(supervisor): run sandbox instance host tests only on Unix hosts The sandbox_instance_host tests added with per-instance supervisor state (NVIDIA#4701) admit hosted instances through SandboxInstanceHost::new, which prepares a private directory per instance. Hosted sandbox instances are Unix-only: on other hosts prepare_private_dir returns "hosted sandbox instances require a Unix host". Prior to this change, the test module still compiled and ran on Windows, so all three tests failed while creating their host. Now, the test module is gated to Unix, and the Unix-only permission check inside one test no longer needs its own cfg attribute. Signed-off-by: Kris Hicks <khicks@nvidia.com> --------- Signed-off-by: Kris Hicks <khicks@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SQLite
put_ifreturned the version of a later writer. UseRETURNING, as Postgres does.Related Issue
Closes #4401
Changes
RETURNINGfor version-checked updates and unconditional upserts.write_result_from_rowhelper.Testing
cargo fmt,cargo clippy --all-targetsandcargo test --libforopenshell-server. The new test fails (duplicate versions) without the change.Checklist