Repository navigation
fix(storage): enforce WAL durability contracts and report unknown write outcomes - #220
Merged
Merged
Conversation
…te outcomes Rebased onto main (9ed61b4), reconciling with request control, API key expiry and the outcome_unknown wire code. Squashes: - fix(persist): exact WAL commit contract across flush and cut failures - no-mistakes(review): Enforce durability barriers and typed unknown write outcomes - no-mistakes(review): Preserve partial WAL outcomes, directory durability, and graph switches - no-mistakes(review): Complete user cleanup and remove redundant outcome variants - no-mistakes(document): Document unknown WAL outcomes and read-only store errors - no-mistakes(document): Add outcome_unknown and store_read_only to GUI WsErrorCode
…rechecks, keep KG drop results
… read-only semantics
…hed`, which enforces that a commit to a shard must use a revision higher than what that shard has already flushed. Otherwise replay would skip the record and an acknowledged write would be lost. Three tests from main in tests/persist_crash.rs broke this caller contract: they committed every write at revision 1, even after a flush (including the flush during reopen-time replay). They now fail with "revision 1 is not after the revisions shard db:r has flushed". The check is correct and required by the intent ("a committed write always survives"), so the fix is in the tests only. What I changed in tests/persist_crash.rs: - `concurrent_appends_and_flushes_lose_no_acked_write`: each writer thread now writes to its own shard `db:r{t}` at rising revisions i+1. Concurrent writers sharing one shard can't keep revisions in commit order, and real callers allocate revisions per knowledge graph. Appends and auto-flushes (buffer_size 7) still run concurrently through the shared WAL, and the test checks that every shard recovers all PER_THREAD writes after a crash. - `concurrent_append_and_delete_recover_what_was_acked`: pre-flushed batches use revisions i+1; the writer uses BATCHES+i+1. - `torn_tail_after_valid_records_is_truncated`: the commit after reopening uses revision 2. Verification: - `cargo test --test persist_crash`: 7/7 pass. - These persistence integration tests also pass: crash_recovery_tests (28), name_validation_tests (11), persist_roundtrip (11), restart_equivalence (8), wal_transaction_recovery (4). - cargo fmt is clean. - Not finished: a background run of the remaining integration binaries had not reported when this result was required. - To make room for the build (the disk was 100% full), I deleted target/debug/incremental and stale test binaries in the worktree's own target/ directory
This was referenced Oct 4, 2026
Closed
jsam
added a commit
that referenced
this pull request
Oct 4, 2026
…ry request (#249) The release-checkpoint performance gate fails on main: cheap_query serial p50 is 1.110x the approved baseline e732cd5 on the 32-vCPU runner (115 -> 126 us). Concurrent latency and throughput did not move. Bisected to #214 (per-connection request pipelining), made worse by #222. Cause: the pipeline runs requests in a `FuturesUnordered`. Its `poll_next` wakes its own task and returns `Pending` after any poll that polled every future in the set. With one request in flight, which is every serial client, that happens on each request's first poll. The connection task wakes itself, tokio schedules that as a yield, and a yield wakes an idle worker to take the task. Each request therefore costs one more context switch (4.0 -> 5.0 per query) and a hand-off of the connection task to another core. #222 added no work per query but made the hand-off dearer: user cycles per query rose 15% at the same instruction count. Fix: keep one never-woken future in the set. After the pipeline's first poll it is never polled again, so no poll reaches every future and the forced yield no longer fires. The yield after futures that wake themselves still applies. Pipelining, ordering, deadlines, cancellation and the credential fence are unchanged. Nothing is added per request: no lock, atomic or allocation. Each connection gets one more set node. Bisect (cheap_query, `?edge(1, Y)`, serial). Server counters per query from perf stat. Ratios are against e732cd5, from an interleaved probe with one pinned server per commit, 40 rounds: | commit | PR | user instr/query | ctx switches/query | p50 ratio | |---|---|---|---|---| | e732cd5 | baseline | 1.282M | 4.0 | 1.000 | | d75ce6e | #208 atomic programs | 1.279M | 4.0 | 0.993 | | 4578fe6 | #215 notification stream | 1.271M | 4.0 | 0.991 | | 71a4d24 | #216 online backup | 1.275M | 4.0 | 1.016 | | 2f621c5 | #200 first-update latency | 1.272M | 4.0 | 1.009 | | e96df5c | #207 | 1.279M | 4.0 | 0.998 | | 4dd8fe9 | #209 shard index | 1.272M | 4.0 | 0.987 | | 277c7f0 | #206 | 1.274M | 4.0 | 1.008 | | 3554c98 | #214 pipelining, deadlines | 1.296M | 5.0 | 1.020 | | 3492a59 | #213 | 1.294M | 5.0 | 1.030 | | 2ba9a5a | #212 provenance | 1.297M | 5.0 | 1.029 | | 9ed61b4 | #204 key expiry | 1.297M | 5.0 | 1.027 | | be35ee3 | #221 | 1.297M | 5.0 | 1.013 | | c98a590 | #222 bounded streaming | 1.298M | 5.0 | 1.047 | | a6fd628 | #219 wildcard fix | 1.297M | 5.0 | 1.061 | | b7f7ad6 | #220 WAL durability | 1.300M | 5.0 | 1.056 | | de54807 | #223 shared standing eval | 1.299M | 5.0 | 1.071 | | 55ec34d | #225 checkpoint-1 fixes | 1.300M | 5.0 | 1.045 | | 23712b8 | main | 1.300M | 5.0 | 1.064 | | this change | | 1.300M | 4.0 | 1.033 | Confirming A/Bs with the gate's own fixture (fresh pinned servers each round, 40 rounds, ratio [95% interval]): - #214 vs its parent 277c7f0: 1.039 [1.027..1.049] - #222 vs its parent be35ee3: 1.025 [1.006..1.044] - A/A 3554c98: 1.010 [1.002..1.025] - e732cd5 -> main: 1.048 [1.028..1.066] - main -> this change: 0.978 [0.958..0.989] - e732cd5 -> this change: 1.021 [1.009..1.044], PASS (budget 1.05) User cycles per query (perf stat, same instruction count): e732cd5 958K, be35ee3 1053K, c98a590 1206K, this change 1006K. Full gate, all fixtures, 20 rounds, servers pinned to CPUs 2-15: - main -> this change: no metric worse. Every required metric is PASS, or INCONCLUSIVE with an interval that includes 1.0. cheap_query p50 0.979 [0.953..1.007]. - e732cd5 -> this change: cheap_query p50 1.029 [1.019..1.072], INCONCLUSIVE on a busy host (load average up to 22); bound_query p50 1.022, PASS. The insert and delta metrics improved since the baseline. The shared host is about 4x slower per query than the CI runner, so the same hand-off is a smaller share of each query here than there. The remaining ~1.7% of user instructions per query since #214 is the pipeline's own per-request bookkeeping: the boxed request future (one 4-8 KiB allocation), its set node, the request control and the in-flight maps.
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.
Intent
WAL durability contracts: a write reported failed never reappears after restart, a committed write always survives, a deleted fact never comes back, and an unknown outcome is reported as unknown with the store failing closed.
What Changed
current.wal.cutmarker (newwal_cut.rs), and the cut is made before the next write or at startup. If the marker can't be saved either, the commit returns a newStorageError::OutcomeUnknown. After that, every write in every knowledge graph is refused withStoreReadOnlyuntil a restart runs recovery. A commit whose record reached the WAL now succeeds even when the flush it triggers fails; the flush failure is logged aspersist_flush_failed.flushed_upperfrontier, and replay skips revisions below it, so changes already in batch files are not applied twice. The frontier only advances after the batch and metadata renames and the directory fsyncs succeed.sync_directorynow returns its errors instead of dropping them, and startup finishes the directory-durability steps before it trusts the frontier. WAL retirement drops only changes that are already durable. Fault-injection tests cover the new paths (durability_tests.rs,wal_tests.rs,program_commit_tests.rs).outcome_unknownandstore_read_only, surfaced by the protocol handler and added tows-protocol, the AsyncAPI spec and the GUI types. The JS and Python SDKs addOutcomeUnknownErrorandStoreReadOnlyError, with tests. The persistence, core-concepts, SDK and WebSocket guides describe the commit outcome rules and the read-only state.Risk Assessment
Testing
Built the target server and drove 9 scenario groups live over loopback WebSockets: acknowledge/delete/restart, failed-write rollback and cut marker, repair retries, flush barriers, unknown outcome via wire/Python/JS SDKs, startup barrier failures and restarts, an unreadable ancestor, read-only refusal across every write family, and the KG-drop ACL cleanup outcomes. Faults were injected only into the server processes with an LD_PRELOAD EIO shim, and the server was always stopped with SIGKILL. All groups passed after three harness-only fixes (wrong rule syntax, a revoke with no entry, and the session switching after .kg create); no product code was changed. Supporting checks also passed: targeted Rust durability, WAL, commit and handler tests (61), Python SDK error tests (22) and JS SDK error tests (21). I removed the disposable .wal-live workspace afterwards. There is no UI surface, so no screenshots were taken.
Evidence: Live driver transcript (groups 1-8)
~/.no-mistakes/evidence/01M41R4738J4AQYJYSAD7GNEX7/run-7836a01/live-transcript.jsonl)Evidence: KG drop cleanup live transcript
Evidence: Startup refused on persist-root parent fsync EIO (first start)
Evidence: Restart with existing root still fsyncs parent and is refused
Evidence: KG drop result kept after ACL cleanup failure
Evidence: Unknown outcome then fail-closed
Evidence: Live harness, fault injector and methodology
Evidence: Targeted Rust test results
Pipeline
Updates from git push no-mistakes
... (5 earlier update rounds omitted to keep the PR body within GitHub's 65536-char limit; full history is in the run log.)
🔧 **Review** - 3 issues found → auto-fixed (5) ✅
🔧 Fix applied.
3 issues (1 error, 2 warnings) still open:
src/protocol/handler.rs:4255- Round 1 introduces an error response that loses an already-applied graph switch. An editor executes.kg create new; the server switches the session, then the automatic owner-ACL write returns OutcomeUnknown. This hunk adds a statement error, but settle_result converts the single-statement result into ProgramError and discards switched_kg. Both SDKs retain the old graph in their cache, so subsequent reads intended for it go to the new graph and fail authorization. Preserve the complete result when an error follows a graph switch. Related sites: handler.rs:4242 (session switch), :5386 (result discarded); JS connection.ts:275 and Python connection.py:278 (switch tracking requires the result field).src/protocol/handler.rs:1365- Round 1 introduced an ordinary-error early exit that skips security cleanup. Drop a user who has API keys and access to a private graph: the user deletion succeeds, then API-key deletion encounters a transient WAL error whose rollback succeeds. This?skips ACL removal. Recreating that username now inherits the old graph access because authorization matches ACLs by username. Related sites: src/protocol/handler.rs:1348 deletes the user, :1382 performs the skipped ACL cleanup, :1304 permits recreation, and :1705 matches the retained ACL. This exceeds the required unknown-outcome propagation. Revert the fail-fast expansion to the minimal fix: preserve typed OutcomeUnknown/StoreReadOnly handling, but remove the ordinary-error early exit so remaining ACL cleanup still runs.src/protocol/error.rs:8- Simplification: Round 1 also extends the unused InputLayerError protocol enum. Repository callers use ProgramError and the WebSocket ErrorCode; InputLayerError has no production consumers beyond its re-export. No stated requirement needs this parallel outcome representation. Remove the added variants here and their conversion arms at src/protocol/error.rs:55–56, retaining the active WebSocket implementation.🔧 Fix applied.
1 warning still open:
src/protocol/error.rs:8- Simplification: Round 1 also extends the unused InputLayerError protocol enum. Repository callers use ProgramError and the WebSocket ErrorCode; InputLayerError has no production consumers beyond its re-export. No stated requirement needs this parallel outcome representation. Remove the added variants here and their conversion arms at src/protocol/error.rs:55–56, retaining the active WebSocket implementation.src/protocol/handler.rs:4122- An applied KG drop can now be reported as an ordinary failure. In this change,cleanup_kg_aclswas changed from best-effort to-> Resultand is called with?after the drop has already committed and its sessions have been closed. Example:.kg drop gsucceeds, then the ACL delete hits a transient WAL error whose rollback succeeds. The request returnsProgramError{code: Internal}and theresultis thrown away. The client is told the drop failed even though it took effect, and a retry gets NotFound. The typed outcome is only needed for OutcomeUnknown and StoreReadOnly. Smallest fix: propagate only those two codes. For any other cleanup error, keep the result and attach a StatementError, the same way the new owner-grant path does at handler.rs:4104-4111.packages/inputlayer-js/tests/statement-errors.test.ts:246- An import insertion went into the wrong place and corrupted an existing assertion.OutcomeUnknownError,andStoreReadOnlyError,were pasted into the argument list ofrejects.toBeInstanceOf(StatementFailedError, ...)in the '.kg use elsewhere / .rel drop x' test.toBeInstanceOftakes one expected class, so the extra arguments are ignored at runtime and fail a TypeScript type-check. Remove lines 246-247. The imports themselves are already correct at lines 20-21.src/storage/persist/mod.rs:1173-create_directorynow fsyncs every ancestor of the persist path up to/on every startup (it is also called again for wal/ from PersistWal::open), and any open or fsync error is fatal. Before this change, directory sync was best-effort. Durable creation only needs the parents of directories this process may have created. If any higher ancestor cannot be opened for reading (for example an execute-only 0711 directory above the data dir) or does not support directory fsync, the server now refuses to start where it used to start. Required intent (review-2) only asks to fsync the parents of the newly created persist root, its subdirectories, and wal/. A narrower option: stop at the first ancestor that existed before this process ran, or tolerate EACCES/EINVAL above the configured root's parent. This is a deployment-policy choice, so it needs confirmation.src/protocol/handler.rs:1348- Simplification: the change adds a second copy of the read-only latch check at the start of each handler. Every write these handlers make already goes through StorageEngine and FilePersist::commit, and those reject with StoreReadOnly at the shared commit boundary (persist/mod.rs:602/616, program_commit.rs:48, storage_engine mod.rs:1364). That shared boundary already meets the fail-closed requirement, so the handler-level pre-checks are not needed. Sites: handler.rs:1348 (user create), 1396 (user drop), 1478 (password), 1531 (role), 1672 (ACL grant), 1748 (ACL revoke), 4423 (ontology install), 4677 (ontology remove), 4814 (ontology upgrade). Recommend removing them and keeping the storage-level checks, including the ones that guard non-WAL file writes (create_knowledge_graph, drop_relation_in, prepare_drop_knowledge_graph).ℹ️
packages/inputlayer-js/src/errors.ts:52- The SDK docs forOutcomeUnknownErrorsay everyoutcome_unknownmeans "store read-only until restart recovery". The server also sendsoutcome_unknownfor two cases that do not latch read-only: CommitError::Unknown (write_run.rs:357) and a panic during commit (supervise.rs:168). In those cases writes still succeed, so the doc misleads callers. The same wording is in packages/inputlayer-py/src/inputlayer/exceptions.py:93. Suggest wording such as "the store may be read-only until restart; a following write raises StoreReadOnlyError if so".🔧 Fix applied.
5 warnings still open:
src/protocol/handler.rs:4122- An applied KG drop can now be reported as an ordinary failure. In this change,cleanup_kg_aclswas changed from best-effort to-> Resultand is called with?after the drop has already committed and its sessions have been closed. Example:.kg drop gsucceeds, then the ACL delete hits a transient WAL error whose rollback succeeds. The request returnsProgramError{code: Internal}and theresultis thrown away. The client is told the drop failed even though it took effect, and a retry gets NotFound. The typed outcome is only needed for OutcomeUnknown and StoreReadOnly. Smallest fix: propagate only those two codes. For any other cleanup error, keep the result and attach a StatementError, the same way the new owner-grant path does at handler.rs:4104-4111.src/storage/persist/mod.rs:1173-create_directorynow fsyncs every ancestor of the persist path up to/on every startup (it is also called again for wal/ from PersistWal::open), and any open or fsync error is fatal. Before this change, directory sync was best-effort. Durable creation only needs the parents of directories this process may have created. If any higher ancestor cannot be opened for reading (for example an execute-only 0711 directory above the data dir) or does not support directory fsync, the server now refuses to start where it used to start. Required intent (review-2) only asks to fsync the parents of the newly created persist root, its subdirectories, and wal/. A narrower option: stop at the first ancestor that existed before this process ran, or tolerate EACCES/EINVAL above the configured root's parent. This is a deployment-policy choice, so it needs confirmation.src/protocol/handler.rs:1348- Simplification: the change adds a second copy of the read-only latch check at the start of each handler. Every write these handlers make already goes through StorageEngine and FilePersist::commit, and those reject with StoreReadOnly at the shared commit boundary (persist/mod.rs:602/616, program_commit.rs:48, storage_engine mod.rs:1364). That shared boundary already meets the fail-closed requirement, so the handler-level pre-checks are not needed. Sites: handler.rs:1348 (user create), 1396 (user drop), 1478 (password), 1531 (role), 1672 (ACL grant), 1748 (ACL revoke), 4423 (ontology install), 4677 (ontology remove), 4814 (ontology upgrade). Recommend removing them and keeping the storage-level checks, including the ones that guard non-WAL file writes (create_knowledge_graph, drop_relation_in, prepare_drop_knowledge_graph).src/protocol/handler.rs:4117- The round-5 F1 fix doesn't change what the client sees, so the problem it was meant to fix is still there. '.kg create' and '.kg drop' must be sent as a single statement (handler.rs:3849-3854), and a drop leaves switched_kg as None. That means settle_result (handler.rs:5280) always turns the StatementError pushed here into Err(ProgramError{code: Internal}) and throws the result away. Example:.kg drop gcommits, then the kg_acls delete hits a transient WAL error whose rollback succeeds. The client is still told the drop failed, and a retry gets NotFound, exactly as F1 described. The owner-grant path only keeps its result because switched_kg is Some there; that precondition never holds for a drop. No test covers this path. Following the recorded instruction literally ('attach a StatementError like the owner-grant path') cannot achieve its stated goal ('keep the drop result'), so the remedy needs a decision. Option 1: report the non-typed cleanup failure without making the response an error, for example as a warning message on the Ok result. Option 2: let settle_result keep a single-statement result whose primary effect was applied. Keep OutcomeUnknown and StoreReadOnly as typed errors either way.src/storage/persist/mod.rs:1175- In round 5 (F3), create_directory now syncs only the parents of directories that did not exist before the call. Concrete sequence: on first start, create_dir_all creates <parent>/persist, then the fsync of <parent> fails and FilePersist::new returns an error. The operator restarts. The root now exists, socreatedis empty, the parent barrier is skipped, and FilePersist::new succeeds without ever making the root's directory entry durable. A commit is acknowledged; after a power loss the whole persist directory, including the WAL holding that commit, can vanish. That breaks the intent 'a committed write always survives'. It also contradicts the earlier recorded review-2 decision to retry the barrier when the directories already exist. The same round weakened the tests to match: durability_tests.rs:290 now makes only one failing attempt for 'ancestor', and the standalone PersistWal retry loop at durability_tests.rs:304 was cut to one attempt. For wal/, the retry gap is covered by the root fsync at mod.rs:176; the gap above the root is not. The later F3 decision ('only directories this process actually created') supersedes the earlier one, so whether this is acceptable is a policy decision. One option that still passes the unreadable-ancestor test: always sync config.path's immediate parent when the earlier attempt could have created the root, for example by tolerating only EACCES/EPERM on that sync. The other is to accept the gap and document it.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
cargo build --bin inputlayer-servercargo test --lib durability -- --nocapturecargo test --lib user_drop_cleanup -- --nocapturecargo test --lib storage::persist::wal::tests -- --nocapture(corrected an initial selector that matched zero tests)cargo test --test wal_transaction_recovery -- --nocapturepackages/inputlayer-js/node_modules/.bin/vitest run --root packages/inputlayer-js tests/statement-errors.test.tsPYTHONPATH=packages/inputlayer-py/src .wal-live/venv/bin/python -m pytest packages/inputlayer-py/tests/test_statement_errors.py -qBuilt the isolated filesystem fault injector withcc -shared -fPIC -o .wal-live/fault.so .wal-live/fault.c -ldl -pthread.Bundled the production JS connection with repository-local esbuild for live SDK execution.Ran.wal-live/live.py,.wal-live/adversarial.py,.wal-live/multi.py, and.wal-live/boundaries.pyagainst isolated production server processes, using real WebSockets, controlled filesystem failures, SIGKILL restarts, and explicit crash-state models.Saved protocol transcripts, SDK exceptions, server fault logs, and reproduction scripts; removed disposable environments, data, and newly generated build outputs. Final git status was clean.✅ No issues found.
cargo build --bin inputlayer-server --bin inputlayer-clientpython live2.py(groups basic, rollback, repair, flush, unknown, startup, unreadable_ancestor, readonly_no_prechecks) against target/debug/inputlayer-server with LD_PRELOAD fault.sopython live2.py kg_drop_cleanup(rerun after harness fix)cargo test --lib -- storage::persist::durability storage::persist::wal::tests storage_engine::program_commit kg_drop_keeps_applied_result user_drop_cleanup read_onlyuv run --with pytest ... pytest tests/test_statement_errors.py(Python SDK)npx vitest run tests/statement-errors.test.ts(JS SDK)🔧 **Document** - 2 issues found → auto-fixed ✅
gui/lib/ws-types.ts:133- The hand-copied WsErrorCode union is missing the new wire codesoutcome_unknownandstore_read_onlydefined in ws-protocol/src/error.rs and docs/spec/asyncapi.yaml. I left it alone because changing it is a GUI type change, not a documentation edit. Suggested follow-up: add the two codes, or generate this type from the AsyncAPI ErrorCode schema so it can't drift again.docs/guides/persistence.md:1- docs/guides/.md are copies of docs/content/docs/guides/.mdx that have drifted apart (persistence compaction settings, core-concepts deletion patterns). As the operator required, this change keeps the WAL contract in both persistence guides, and the stale Programs Commit Together section in core-concepts.md now just links to the .mdx version. Suggested follow-up: reduce the docs/guides copies to pointers or remove them, so each fact lives in one place.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.