Repository navigation
release: v0.1.4 — three correctness fixes, a pruning proof, and five documents made true - #58
Merged
Merged
Conversation
`submit_one` pushed an SQE and then returned early if `submit_and_wait` failed. After a successful push the SQE belongs to the kernel, so that path dropped `read_range`'s buffer while the kernel could still be writing into it, and left the completion to be reaped by the *next* job, which read the stale `cqe.result()` as its own. `user_data` was a constant per operation kind, so nothing could tell the two apart. EINTR is not a failure: any signal delivered to the ring thread produces it, and the SQE is still in flight when it does. It is retried now. Other errors are recorded and reported only *after* the completion is in hand, never instead of waiting for it, so no path returns while the kernel may still write. If a submission the kernel accepted somehow never completes, the assert fires rather than the buffer being freed underneath it — unreachable with one SQE in flight and a completion queue twice the submission queue, which is why it is an assert and not an error. Every SQE now carries the worker's next submission number, and a completion with any other tag is a hard error: the ring and the worker disagreeing about what is outstanding makes every result from it untrustworthy, including that one. The regression test aims rather than sprays. An end-to-end read was the first attempt and it was useless: a local read completes in microseconds, so a signaller racing it never lands inside `submit_and_wait`, and the test passed against the bug. It reads from an empty pipe instead, which cannot complete until the test writes to it, so the worker is certainly blocked when `tgkill` — this thread, not the process — delivers five SIGUSR1s. The handler is installed without SA_RESTART, or the kernel restarts the syscall and there is no EINTR to survive. Reverting `submit_one` makes it fail; that is checked, not assumed. `libc` joins the workspace manifest as a dev-dependency for the handler and `tgkill`. It was already in the graph transitively, so nothing new is built or shipped. Closes #27 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
`UringLocalFileSystem::get_opts` read `range` and `head` and ignored everything else, while `LocalFileSystem::get_opts` calls `check_preconditions` before reading a byte. The two stores are documented as behaving identically, so `if_match`, `if_none_match` and `if_modified_since` were honoured or silently dropped depending on which store a caller happened to hold — a conditional read succeeding where it should have been refused. The precondition cases go in `exercise`, the shared conformance body, so both stores are held to them rather than only the one that was wrong. Removing the check again makes it fail on "if_match on a stale etag is a precondition failure"; that is checked, not assumed. The job channel is bounded. The worker handles one SQE at a time, so an unbounded queue did not add throughput — it moved the backlog out of the callers, where it would have shown up as backpressure, and into memory, where it was neither visible nor limited. Refs #44 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
`oxide sql … | head -1` exited 101 with "failed printing to stdout: Broken pipe". `DataFrame::show()` is `println!` inside DataFusion, and `println!` panics when the reader has gone — so the most ordinary thing in a shell pipeline crashed the primary way a query tool is used, against SECURITY.md's no-panics bullet and ADR-0008. `sql` formats with arrow's pretty printer (through DataFusion's re-export, so no new dependency) and writes by hand, as do `explain` and `gen-data`. `BrokenPipe` is then a value to match on rather than a panic raised inside the formatting machinery, and it means success: the reader got what it asked for. The test took two attempts and the first one was worthless. With 5,000 rows the whole table fits in a pipe buffer, the writer finishes before `head` exits, and no EPIPE is ever raised — so it passed against the panic. Measured upwards: 20,000 rows reproduce it every time, and the test uses 50,000. `assert_cmd` cannot express this at all, since it captures stdout itself and never closes it early, so the pipeline is built by hand. The `explain` case is marked in the file as *not* a regression test for the panic — its output fits a pipe buffer whatever the data — so nobody later reads it as covering more than it does. Closes #34 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The README says Parquet is pruned "by DataFusion's statistics, page index and Bloom filters … with proofs", and two of the three had one. `page_index_rows_pruned` was already reported by `PruningSummary` and never asserted anywhere. The proof needs a dataset the other two mechanisms cannot touch, or it restates them. `seq` is sorted across the whole file and written as a single row group, so min/max statistics cannot exclude that row group for any predicate inside its range, and there is no Bloom filter. A 1,000-row range then prunes 198,192 of 200,000 rows with `row_groups_pruned() == 0` — every one of them the index's work. The pruning-disabled run prunes none, and both runs return the same answer. `ParquetWriteOptions::data_page_rows` is what makes that observable: the page index prunes at page granularity, so pages have to be small enough for a narrow predicate to skip most of them. `None` keeps the parquet crate's byte-based default, which is the right choice for real data. Closes #43 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Three documents gave two answers to the most important verification question. README said the CUDA backend "has not yet run on a CUDA machine"; STATUS's matrix has recorded since 2026-09-06 that an AWS `g4dn.xlarge` T4 NVRTC-compiled and launched every kernel, exercised pinned transfers, and is where the aggregation kernel's correctness bug was found. ADR-0012 makes STATUS the ledger, so under-claiming is as broken an audit trail as over-claiming: a reader who checks the headline concludes the GPU path is unproven when it is the better-evidenced half. - README now says both backends have executed on real hardware and points at the matrix, naming the T4 and the date. - STATUS's "Both compile-verified only — no CUDA machine has run this code" bullet went stale the same day and is gone; the paragraph it sat in now points at the matrix. - The header said 0.1.1 was the current follow-up while 0.1.3 had shipped, and dated itself 2026-09-06. Refreshed, and it now says what "v1 complete" means — the internal phase plan of docs/SPEC.md, not a 1.0 release — because "v1 complete" beside "0.1.3" reads as a contradiction to a first-time visitor. - README leads with "query engine" and says plainly that a catalog, a metastore and transactions are out of scope, so "lakehouse" is qualified rather than claimed. The 0.1.0 CHANGELOG entry keeps its historical CUDA note: it was true when it shipped, and editing a released entry would be rewriting the record this issue exists to protect. Closes #36 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
`UringLocalFileSystem` is implemented, conformance-tested and constructed by nothing: every session registers the default `LocalFileSystem`, so `--features io-uring` changes nothing at run time. Three documents said otherwise — SPEC §2.4 claimed it was "registered into the session's `RuntimeEnv` so Parquet scans and spill IO both use it", roadmap Phase 4 ticked that line, and the README repo map listed it as a storage feature without qualification. A user who turns the flag on gets neither io_uring nor an error. Demoted rather than wired, deliberately. The 2026-09-02 audit measured the ring *slower* than `LocalFileSystem` on this workload, so registering it now would be a regression wearing a feature's clothes. The roadmap line is unticked with the reason and the blocking measurement named, so the work is still tracked rather than quietly dropped. Refs #24 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
architecture.md and SPEC §2.1 both said host buffers are page-locked with CUDA active, "DMA without staging copies". The operator path does not do that: `upload_bytes` copies from a pageable Arrow `Buffer` and `download_column` copies into a pageable `Vec`. `alloc_pinned_host` and `PinnedBuf` exist and work — the T4 tests exercise them — but nothing in `Gpu*Exec` calls them. The distinction matters because a transfer-throughput number read off the documented DMA path would be wrong by exactly the staging copy it claims to avoid, so both documents now say which number a measurement today actually is. Pinned staging for operator transfers stays tracked as audit item P6. Closes #26 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
One ticked box was not true. Phase 6 claimed the GPU operators run "with double-buffered stream pipelines"; `round_trip` does upload → kernel → download → `synchronize` per batch, serially, and the audit lists double buffering under Deferred. ADR-0012 makes the roadmap an auditable record, so a tick the audit contradicts is exactly the drift that rule exists to stop. It now says what the code does and where the overlap went. The roadmap otherwise ended at a fully ticked v1, with the forward-looking work split between a milestone and `docs/audit-2026-09-02.md`. Phase 8 brings them into one list: every open issue in `v0.2.0` under Correctness, Performance, Operability, and Supply chain and release, each with the acceptance criterion that would close it, and the audit's deferred plans folded in by issue number rather than left in a second document to drift from this one. Lines already shipped are ticked with the release that shipped them, so the section is a record and not only a plan. STATUS's header points at it. Closes #23 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
SECURITY.md and ADR-0016 said "no build cache"; three jobs have used `Swatinem/rust-cache` since ADR-0017. The audit that motivated that exception asked for a cold/warm comparison before any saving was claimed, and it had never been made — so the exception was undocumented *and* unjustified, and there was no way to tell which document was wrong. Measured first, from the API, seven successful `ci.yml` runs on main with queue time included: 7 min warm in five of five runs, 19–29 min cold when the lockfile moved, against the 59 min pre-cache median. Roughly 8x on the common path and 2–3x on the worst one. So the exception keeps its place and the documents move instead. What matters for the posture is not that a cache exists but what it can serve: `cache-bin: false` and `cache-workspace-crates: false` mean no workspace crate and no tool binary ever comes back, and `github.workflow == 'CI' && !inputs.clean` means a release `workflow_call` restores nothing. "No build cache that could serve a stale artifact" was true about the artifact and wrong about the mechanism; SECURITY.md now says which, ADR-0016 records that ADR-0017 amended it, and verification.md — already accurate — points at the numbers. docs/ci-cache-measurement.md carries the run URLs and says what would void it: a closing cold-run margin, or a cache extended beyond dependencies. Closes #39 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
vyncint
force-pushed
the
fix/correctness-sweep
branch
from
September 20, 2026 13:44
a10056d to
e3f0008
Compare
The default Linux lane ran `cargo install termlens-cli` from source on every run, and `cache-bin: false` meant it was never reused — minutes of compilation for a binary the lockfile already pins exactly. Every other tool in this CI arrives prebuilt (cargo-deny, cargo-semver-checks, zizmor); this is the rule for the ecosystem's workflows, prebuilt tools rather than cached builds. `taiki-e/install-action` fetches it at the version read out of `Cargo.lock`, so the tool and the library stay one release, and the lane passes `TERMLENS_CLI` to the suite. The action is pinned to 94c31af3204a9f15ab40b35ad084410b905bbc73 (v2.87.17), looked up with `gh api repos/taiki-e/install-action/git/ref/tags/v2.87.17` rather than copied. The shortcut has one failure mode: a prebuilt install at the wrong version silently tests a different tool. So the test does not trust `TERMLENS_CLI` — it runs `--version` and refuses anything that is not the version the lockfile names, which also keeps the local `cargo install` path and the CI path honest about the same thing. Closes #48 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Three correctness fixes (#27 io_uring use-after-free, #44 ignored GetOptions preconditions, #34 panic on a closed pipe), a third pruning proof (#43), a prebuilt termlens-cli in CI (#48), and five documentation issues that had the repository claiming things the code did not do (#23, #24, #26, #36, #39). Ten issues from the v0.2.0 milestone. Each correctness fix landed with a regression test that was checked to fail without it -- two of the three tests were worthless on the first attempt and were rewritten after the revert-and-rerun said so. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
vyncint
changed the title
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
1 participant
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.
Ten issues from the
v0.2.0milestone, then the release.Correctness
submit_onereturned early whensubmit_and_waitfailed — but after a successful push the SQE is the kernel's, so that freed the caller's read buffer under it and left the completion for the next job to reap as its own. EINTR is retried, other errors are reported only after the completion is in hand, and every SQE carries a submission tag.UringLocalFileSystem::get_optsignoredif_match/if_none_match, whichLocalFileSystemhonours — a conditional read behaved differently depending on which of two "identical" stores you held. Job channel bounded too.oxide sql … | head -1exited 101 fromprintln!inside DataFusion'sshow().Two of the three regression tests were worthless on the first attempt, and only reverting each fix and re-running found that out:
submit_and_wait. It reads from an empty pipe now, which cannot complete until the test writes, withtgkillaimed at the waiting thread and a handler installed withoutSA_RESTART.EPIPEwas raised. Measured upward: 20,000 reproduce it reliably; it uses 50,000.Added
#43 — the third pruning proof. It needs a dataset the other two cannot touch or it restates them: a sorted column in one row group, where statistics cannot exclude the row group and there is no Bloom filter. 198,192 of 200,000 rows pruned with zero row groups pruned.
ParquetWriteOptions::data_page_rowsis what makes it observable.Documents that claimed things the code did not do
#36 README said CUDA "has not yet run on a CUDA machine"; STATUS has recorded a T4 run since 2026-09-06 — including where the aggregation kernel's bug was found. #24 SPEC said the io_uring store was registered into the session's
RuntimeEnv; nothing constructs it. Demoted rather than wired, because the audit measured the ring slower. #26 pinned memory is available but not what the operators use. #23 Phase 8 added; the "double-buffered stream pipelines" tick removed, sinceround_tripis serial. #39 measured the CI cache before deciding: 7 min warm, 19–29 min cold, against a 59 min pre-cache median — caches stay, documents corrected.#48 termlens-cli installed prebuilt at the lockfile's version; the test refuses a binary whose
--versiondisagrees.Not done, and why
#47, #49, #32 remain open. #32's acceptance needs a Metal device. #47 and #49 are feature work I stopped short of rather than land partially next to a release. The CUDA issues (#28, #29, #45, #46), the architectural ones and the three blocked on upstream Ballista are out of this pass.
Gate
cargo test --workspace --locked36 result lines / 0 failures · clippy-D warnings· fmt ·cargo deny checkall four ok · crate-metadata · skill-version. Version bumped across all ten crates and the lockfile;extract-changelog.sh 0.1.4prints 170 lines.