Repository navigation
fix(projection): avoid TEMP output ownership preflight during rebuild - #4802
ScriptedAlchemy wants to merge 7 commits into
Conversation
|
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Reviewed the #4152 preflight change at b14ced8 against the production staging and raw-message storage paths. The regression has a concrete fixture mismatch below; changing the TEMP page-count baseline does not address it. This is a source-level finding, not a claim that I executed the Rust test. After correcting the fixture, run the focused regression through Bazel with a confirmed nonzero test count, then the affected global-db tests on the published head. Keep this PR as Refs #4152: passing these checks would not by itself prove the original in-memory corruption cause or all safe-reclamation requirements.
|
|
||
| // Matching foreign bodies are compatible; rejecting every unowned row | ||
| // would also avoid spills, but would break legitimate rebuilds. | ||
| write_staged_message(&transaction, GENERATION, &foreign) |
There was a problem hiding this comment.
[P1] Stage the canonical stored representation, not the original input record. The earlier upsert_projection_raw_message(&foreign, ...) calls projection_stored_message, which writes non-null metadata_json containing ingest-protection receipts even though this fixture starts with metadata_json: None (protected_metadata_json returns Ok(Some(...))). write_staged_message only serializes its argument, so this call stages null metadata. For provider cursor, the preflight explicitly compares active.metadata_json IS NOT json_extract(staged.message_json, '$.metadata_json'); it therefore returns OutputCollision even when the body is unchanged. The following expect("unchanged foreign output must remain eligible") fails before the TEMP-storage, rollback, and integrity assertions run. Stage the changed and compatible cases through the same projected_stored_message / projection_stored_message normalization used by production rebuild staging, and retain the collision and storage assertions. Do not weaken the production metadata comparison to accommodate the fixture. The page-count adjustment in b14ced8 cannot fix this mismatch.
|
Superseded by issue-specific #4812, a clean single-commit fast-forward from master with the identical reviewed source/test bytes at |
Root cause and scope
Refs #4152.
prepare_rebuild_output_activationmaterialized one TEMP ownership table for every staged output on the registered SQLite writer, then dropped/recreated it at each rebuild activation. Those TEMP tables are backed by SQLite's process-local temporary storage and can leave sizable unlinkedetilqs_*files open on a long-lived writer even after a table is dropped. This is distinct from the two ownership TEMP tables addressed by the partial commit currently held on DRAFT #4776.Replace that materialization with an indexed, correlated collision preflight over canonical active messages, current/foreign projector ownership and staged creation evidence. Run the check before clearing projector provenance, preserving the original protections: reject absent outputs without creation evidence; reject modified foreign-owned or unowned outputs; allow unchanged foreign outputs and updates solely owned by this projector. No TEMP table is created by this preflight, and the existing output row is left unchanged on refusal.
Real behavioral regression
An
#[asupersync::test]on the registered global DB exercises the production preflight through its SQLite executor:OutputCollision;observation_projection_rebuild_preexisting_outputsinsqlite_temp_master(this assertion fails on the previous implementation);PRAGMA integrity_check.An independent disposable SQLite 3 comparison passed 32 combinations of active output, current ownership, foreign ownership, staged creation and equal/different body, matching the old and new conflict classifications.
EXPLAIN QUERY PLANshowed indexed probes and no TEMP B-tree on the replacement query. This is supporting evidence, not a repository Rust/Bazel test pass.Verification and boundaries
Pending:
bazel test //crates/tracedecay-global-db:unit_test --test_output=errorswith a confirmed nonzero execution of the new regression; exact-head hosted CI, native macOS coverage, and review. Bazel is unavailable in the execution environment; no Cargo was used.This is a partial fix, not a claim that #4152's original in-memory SQLite corruption cause has been proven, nor that all query sorts and process temp-spill files are gone. The other ownership-TEMP removal on mixed DRAFT #4776 is deliberately untouched; the application integration and Kev/decision PRs must remain DRAFT and must not merge or auto-merge. No production/infra/release changes. Do not close #4152 until its remaining acceptance evidence exists on
master.Committed-data / TEMP-storage regression hardening (c178ff5)
The preflight regression now seeds the foreign body in a committed transaction before starting the rebuild. On refusal it explicitly rolls the rebuild back, opens a new read snapshot and asserts that the committed body, its original location and the missing staging generation are unchanged, then runs SQLite
PRAGMA integrity_checkon that snapshot. On the long-lived writer it also asserts thatPRAGMA temp.page_countdoes not increase over its same-connection preflight baseline, not just that thesqlite_temp_masterrow is absent: in an independent SQLite probe, creating a small TEMP table raised TEMP pages from 0 to 2 and dropping it left 2 allocated pages. The old implementation leaves a named TEMP table visible; the page-count comparison separately detects new persistent TEMP allocation without assuming that unrelated connection initialization allocated zero pages.The earlier all-green draft workflow did not execute the Linux Rust tests: the repository skips native Rust CI on draft PRs. This issue-specific PR was marked ready for review, without enabling auto-merge, to request the full Bazel CI on its exact head. Until those jobs complete, no Rust or native/macOS pass is claimed.
CI test follow-up (b14ced8)
The previous exact-head Linux fleet job reported this new Rust test as FAILED, but the full 499-test target hit its 300-second timeout before libtest printed the individual assertion failure. The fleet also reported unrelated automation scheduler failures and other timeouts; these are not evidence of a successful #4152 test.
The new test commit compares the writer's TEMP page count before and after preflight instead of demanding absolute zero. That keeps the storage regression sensitive to a new persistent allocation without conflating unrelated TEMP pages that can already exist on the long-lived connection. The new exact-head Bazel CI is still pending; no Rust test pass is claimed.