Conversation
On-disk SQLite stores ran with sqlx defaults: rollback journal (`journal_mode=delete`) and `synchronous=FULL`. Every autocommit write paid several fsyncs and blocked readers while it held the lock, so gateway hot paths made of many small writes serialized on disk latency. The clearest case is `openshell forward service`, which mints and revokes an SSH session token around every forwarded TCP connection: two commits per connection, tens of milliseconds each on a virtual disk, wall clock linear in the number of concurrent connections, and enough queueing that bursts hit the per-sandbox connection cap and get refused. Switch on-disk databases to WAL with `synchronous=NORMAL`. The mode change runs once on a single connection before the pool opens: entering WAL needs exclusive access to the file, so doing it up front means pool connections only ever re-apply the pragma to a file already in WAL mode, and a failure surfaces as one clear connect error. The first start after upgrading an existing database therefore needs the file to be otherwise unopened. `synchronous` is applied through the connect options on every pooled connection. In-memory databases keep their defaults. A crash can now roll back the most recent transactions without corrupting the database, which is the standard WAL trade-off and fits the single-node scope of the SQLite backend. Tests cover a fresh store, an existing rollback-journal file that must be switched on connect, sidecar permissions, and concurrent readers under a burst of insert-then-update writes. Architecture, configuration and Helm docs describe the durability trade-off, the sidecar files, and the local filesystem requirement. Signed-off-by: Jason T. Greene <jason.greene@redhat.com>
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
|
/ok to test 5cced0e |
|
Label |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The accepted SQLite concurrency fix is project-valid, its operator-facing durability and storage constraints are documented, and the current required checks including E2E are green. One security-durability blocker remains before pipeline handoff.
Action required: Preserve durable acknowledgement for revocations and other authorization-reducing writes, then add a recovery regression test.
Blocking findings:
GATOR-5cced0e3-01: WAL withsynchronous=NORMALmay lose an acknowledged security revocation after power failure.
Carried findings:
- None
Non-blocking suggestions:
- None
Gator metadata
- Validation: Fixes accepted issue #3494 with a focused SQLite persistence change
- Docs: Fern gateway configuration, architecture, Helm, and RPM documentation updated
- Checks: Current-head required checks are green
- E2E:
test:e2eapplied; OpenShell / E2E is green for the current head - Head SHA:
5cced0e39e74c5fa317b4a07e8602526158e4216 - Base SHA:
24706c175b25574640cb5e0d508ec063d4320770 - Merge base SHA:
24706c175b25574640cb5e0d508ec063d4320770 - Patch ID:
62bb61f4dc0a5708cdce40a174489fc22c80083c - Gator payload:
9 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
WAL with synchronous=NORMAL can roll back acknowledged commits after a power loss or kernel crash, including SSH session revocations and other authorization-tightening writes. Run the main pool with synchronous=FULL so every acknowledged write is durable; in WAL mode that is a single fsync of the WAL per commit. Add Store::create_relaxed for inserts that are safe to lose, and use it only for SSH session issuance: a dropped token just fails validation. On file-backed SQLite it runs on a dedicated single-connection pool with synchronous=NORMAL. Both pools share one WAL, so the next FULL commit also makes earlier relaxed commits durable. Postgres treats it as an ordinary durable MustCreate insert. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
|
/ok to test d5737da |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @mrunalp. I checked your durability update against the prior finding: authorization-reducing writes now use the synchronous=FULL pool, only fail-closed SSH session issuance uses the relaxed pool, and the settings tests pin that separation. No blocking findings remain, and the current-head Branch Checks and E2E workflows are running.
Blocking findings:
- No blocking findings remain
Carried findings:
GATOR-5cced0e3-01: Resolved by the current head; the Gator review thread is now resolved
Gator metadata
- Validation: Fixes accepted issue #3494 with a focused SQLite persistence change
- Docs: Fern gateway configuration, architecture, Helm, and RPM documentation updated
- Checks: Current-head required checks are queued, running, or complete; Branch Checks remain pending
- E2E:
test:e2eis applied and the current-head E2E workflow is running - Head SHA:
d5737da6edefc6ae53c0c430ea11e7f378a397aa - Base SHA:
24706c175b25574640cb5e0d508ec063d4320770 - Merge base SHA:
24706c175b25574640cb5e0d508ec063d4320770 - Patch ID:
a870b525e3ae5f1cada7dc7cfc0aca65af8391b3 - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
5cced0e39e74c5fa317b4a07e8602526158e4216 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
|
Re-ran the forward-service sweep against the current head ( |
Summary
On-disk SQLite stores ran with sqlx defaults (rollback journal,
synchronous=FULL), so every autocommit write paid several fsyncs and blocked readers.openshell forward servicedoes two such writes per forwarded TCP connection (session token minted and revoked), which made connection setup through a forward linear in the number of concurrent connections and pushed bursts into the per-sandbox connection cap. This switches on-disk stores to WAL. Commits stay atsynchronous=FULL(one WALfsynceach) so acknowledged writes such as SSH session revocations survive a power loss; only SSH session issuance, whose loss just invalidates a token, runs atsynchronous=NORMAL.Related Issue
Fixes #3494
Changes
SqliteStore::connect: for on-disk URLs, switch the file tojournal_mode=WALonce on a single connection before the pool opens (entering WAL needs exclusive access; done up front so pool connections only ever re-apply the pragma to a file already in WAL mode and a failure surfaces as one clear connect error), then build the main pool withjournal_mode=WALandsynchronous=FULL, plus a single-connectionsynchronous=NORMALpool for relaxed writes. Both pools share one WAL, so the next FULL commit also makes earlier relaxed commits durable. In-memory databases are unchanged and use one pool. A failed switch reports the file path and the exclusive-access requirement.Store::create_relaxed: MustCreate insert that is allowed to be lost in a crash. On file-backed SQLite it runs on the NORMAL pool; on Postgres it is an ordinary durable insert.handle_create_ssh_sessionuses it; revocation and every other write keep the durableput_ifpath, so relaxed durability is opt-in.persistence/tests.rs): fresh on-disk store reportswalwithsynchronous=2on the main pool andsynchronous=1on the relaxed pool, and its-wal/-shmsidecars are 0600; an existing rollback-journal database is switched on connect;create_relaxedrejects duplicates and its insert can be revoked through the durable pool; concurrent readers proceed under a burst of relaxed-insert-then-durable-update writes on a file-backed store (so both pools contend for the writer lock); the stale comment about non-WAL production is reworded.architecture/gateway.md(durability settings and the relaxed issuance path, backup withsqlite3 .backup/VACUUM INTO, local filesystem requirement),docs/reference/gateway-config.mdx,deploy/rpm/CONFIGURATION.md, Helmvalues.yaml/README note that the SQLite volume must be local block storage.Out of scope, noted for follow-up: an explicit store close on gateway shutdown for a final checkpoint; minting one session token per forward process instead of per connection; making the per-sandbox forward connection cap configurable.
Testing
cargo fmt -p openshell-server -- --check,cargo clippy -p openshell-server --all-targets --features test-support -- -D warnings,python3 scripts/update_license_headers.py --checkall clean on the rebased branch (main @ the base of this PR).cargo test -p openshell-server --features test-support persistence: 85 passed, 0 failed, 3 pre-existing ignores; the three new tests were also repeated 40 times with--test-threads=8without a failure.mise run pre-commitclean after the durability follow-up commit; SQLite persistence and SSH session tests (33) pass.mise run cinot run locally.Before/after with the same source, gateway on a hosted runner with the Docker driver, a sandbox serving loopback HTTP,
openshell forward servicein front, N simultaneous connections each doing one request (reproducer and scripts: https://github.com/n1hility/OpenShell/tree/forward-sweep-repro, workflowforward-sweep.yml; details on #3494):Re-measured on the current head (
d5737da6: WAL,synchronous=FULLon the main pool, relaxed issuance) with the same reproducer, both sets on oneubuntu-latestrunner (fdatasync p50 about 0.4 ms; run: https://github.com/rh-forge/openshell/actions/runs/35930454359, patched gateway built from a checkout carrying this PR's two commits on the same base as the baseline image):On this disk the head is indistinguishable from the first commit's all-NORMAL numbers (0.056 s at 32, 0.089 s at 64 in the earlier run). The difference appears on slow-fsync storage: each closed connection's revocation now holds the SQLite writer lock for one WAL
fsync, and a new connection's issuance queues behind it, so burst time grows linearly with concurrent connections at roughly onefsynceach. A store-level harness with a 4.5 msfsyncmeasured about 9 ms per connection for the head versus 35-50 ms for main and under 1 ms for the all-NORMAL variant; on a virtio volume with 8-12 msfsyncthat projects to about 0.5 s at 32 connections and 1.2 s at 64, against 3.3 s and 6 s before this change. That is the durability trade made in the follow-up commit; removing the per-connection write altogether (one session per forward process) is the follow-up noted on #3494.On a 2 vCPU VM with SQLite on a network block volume the same sweep went from 1.52 s to 0.06 s at 16 connections and from 6.05 s to 0.12 s at 64. Remaining refusals above 20 connections are the fixed per-sandbox cap, independent of this change.
Checklist