Reject a recorded type byte that names no type - #407
Conversation
A persisted entry type was cast to NodeType and a persisted edge type to LinkType without checking either against its enum. NodeType names 0-10, so a recorded byte of 11-255 became a well-defined value no enumerator names; it then fell through prior_paths' deliberately default-less switch into none of sources, generated, or unowned, and fed the overwrite guard as a path classified as nothing. The switch's totality comment was true of the switch and false of the function. The edge side was the same shape with a worse tail: LinkType names 1-5 and has no zero enumerator, so both 0 and anything above 5 produced LinkRole::Unknown, which joins no mask and therefore routes nothing — a recorded dependency silently ceasing to route is the failure class the incremental campaign exists to prevent. An unnameable type byte is damage, not a value from a future version. Tolerating it has no disposition a reader can implement: dropping the entry is the silent-nothing state itself, and substituting a classification is the substitution this area already forbids. Forward compatibility does not apply either, because the readable window's floor admits only versions whose type vocabulary is a prefix of this one's. Enforce at the cast site so no entry carrying an unnameable type is ever constructed: FileEntry::from_raw and EdgeEntry::from_raw now return Result and report IndexDamaged, which the announcement side already covers. Observers are untouched, no default: arm is added anywhere, and the totality comment stands unchanged because the fix makes it true. names_node_type switches exhaustively with no default, so adding a NodeType enumerator fails -Wswitch rather than silently widening what the reader accepts. names_link_type delegates to link_role instead of restating the valid range, keeping one source of truth for which link types exist. The recovery read (read_prior_paths, #291) reports the record as lost on a damaged byte rather than proceeding with a partial table — no prior knowledge is strictly safer than knowledge that omits entries. Verified: RED quoted for both legs before any production code. build, format, tidy, iwyu, spec-check all exit 0; [index] tag 0 with 7356 assertions. The full suite is green when carried onto the #397 base; on main it fails only in test_builder.cpp:87, which makes scratch under a read-only /tmp on this machine and is exactly what PR #406 fixes. This change does not touch that file. Fixes #399
|
Holding this as a draft — a third leg belongs in it before it merges. The architect ruled the persisted flags word CLOSED, making it part of this issue rather than a follow-up. I verified both load-bearing claims at the line before accepting:
The builder's original observation was right on its own terms and is confirmed as fact — every consumer is a Planned addition, deliberately small:
The spec requirement will be worded over the class — a recorded value outside the vocabulary its version defines — so no flags-specific carve-out sentence is ever needed. No merge until that lands. The merge decision is the user's regardless. |
PR metricsPerformance (gcc example, Linux)
Deterministic signals: instructions (cachegrind-simulated instruction reads — exact across runs, no PMU needed), page faults, peak RSS, and the cachegrind D1/LL miss rates. CPU time is user+sys from time(1). Internal statistics (gcc example, up-to-date dry run)
Counters from Binary size (Linux)
Code churn (whole codebase, last 30d)
Of the lines written across the codebase in the last 30 days, how many are already gone — work that was written and then discarded or rewritten inside the same window. This is the state of the tree including this PR, not a measure of the PR itself. Only code we write is counted: tests, examples, vendored and generated files, CI plumbing and prose are excluded. 4362 lines were deleted in the window in total, most of them older than it. Where the churn is
Test coverage (lines)
102 files · 16646/18886 lines covered Deltas vs main@26ddb6105. Updated for a611bb4 |
|
CI is 13/13 green on this branch as it stands (run against base Worth recording because it settles the branch-base question this PR was opened under: the local So this branch did not need to stack on #406, and it does not. The two PRs are independent. This PR nonetheless stays a draft until the flags-word leg lands, per the ruling recorded above — green CI is not the same as complete. |
The persisted node and command flags words were read back with no check against their vocabularies. NodeFlags names bits 0-4 and CommandFlag bit 0, but get_node_flags reconstructs the whole 16-bit word and set_node_flags writes the whole word back, neither masked -- so a garbage high bit read today is written into a fresh, re-checksummed record and thereafter looks authentic. An unnamed bit is not inert; it carries. Openness buys nothing to offset that: read_index opens at exactly INDEX_VERSION, so no record it reads can carry a legitimately newer flag vocabulary. Validate at the cast site, where the type bytes are already validated: FileEntry::from_raw rejects a node flags word with a bit outside the readable mask, and CommandEntry::from_raw -- now returning Result -- rejects a command flags word the same way. Neither mask is written out. RECORDED_NODE_FLAGS_MASK is derived by a constexpr bit sweep over an exhaustive is_recorded_node_flag switch with no default arm, so a new flag that forgets the mask is a -Wswitch build error rather than a reader that silently rejects a legitimate record. The command mask derives from the existing flag_category for the same reason, minting no second classifier; that made flag_category's "nothing calls this" comment false, so it is gone. The vocabulary a word is judged against spans every version the readable window admits, uniformly rather than per-version -- the check takes no version and branches on none. NodeFlags::Inactive held bit 5 through index version 13 and was retired without a format change, and read_prior_paths reads back to INDEX_LAYOUT_FLOOR, so bit 5 stays readable: validating against today's live mask alone turns a legitimate v9-v13 record into Lost. A read word is also carried whole into the next record written, so a current-version record holding a retired bit is an honest descendant of a pre-retirement one rather than damage. Bit 5 is now a hole, and a sixth flag would naturally take it -- at which point an old record's stale Inactive bit would silently mean the new flag, which is misinterpretation rather than the loud over-rejection this fix started from. A disjointness static_assert makes reassigning a retired bit a build error, and the value pin's message routes a future retirement into the retired mask instead of letting the bit vanish. Both from_raw doc headers now state their rejection contracts, which neither did for the failure modes this change gives them: FileEntry's named only the type byte, and CommandEntry's said nothing at all on a function that had just gained a failure mode. Verified: RED quoted for both legs before any production code; the retired bit is pinned by a test proved to fail against the live-mask-only shape, and the disjointness assert was proved to fire by reassigning a retired bit. build, format, tidy, iwyu, spec-check all exit 0; whole suite green in one process, 170265 assertions in 841 test cases.
1 participant
What was wrong
Two persisted bytes were cast straight to their enums without validation.
Entries.
NodeTypenames 0–10, so a recorded byte of 11–255 became a well-defined value noenumerator names. It fell through
prior_paths' deliberatelydefault:-less switch into none ofsources, generated, or unowned, and then fed the overwrite guard — a path classified as nothing. The
switch's totality comment was true of the switch and false of the function.
Edges. Same shape, worse tail.
LinkTypenames 1–5 with no zero enumerator, so both0andanything above
5producedLinkRole::Unknown, which joins no mask and therefore routesnothing. A recorded dependency that silently stops routing is the failure class the incremental
campaign exists to prevent.
Why damage rather than forward compatibility
Tolerating an unnameable byte has no disposition a reader can implement. Dropping the entry is the
silent-nothing state being indicted, and substituting a classification is the substitution this area
already forbids. Forward compatibility does not apply either: the readable window's floor admits
only versions whose type vocabulary is a prefix of this one's, so a byte outside today's enum cannot
have come from a readable older version.
The fix
Enforced at the cast site, so no entry carrying an unnameable type is ever constructed.
FileEntry::from_rawandEdgeEntry::from_rawreturnResultand reportIndexDamaged.Deliberately unchanged:
default:arm is added anywhere.IndexDamagedis already handled, so nothing widened.reworded.
Two encoding choices worth noting:
names_node_typeswitches exhaustively with nodefault:, soadding a
NodeTypeenumerator fails-Wswitchinstead of silently widening what the readeraccepts; and
names_link_typedelegates to the existinglink_rolerather than restating the validrange, keeping one source of truth for which link types exist.
The recovery read (
read_prior_paths, #291) reports the record as lost on a damaged byte ratherthan proceeding with a partial table — no prior knowledge is strictly safer than knowledge that
silently omits entries.
Spec
New requirement
REQ-READ-REJECT-UNKNOWN-TYPE, discharged by three tests — entry type, edge type,and the easily-missed edge-type-zero case, which is invalid precisely because
LinkTypehas no zeroenumerator.
Verification
RED quoted for both legs before any production code was written.
[index]The full suite is green when this work is carried onto the #397 base. On a
mainbase it fails onlyat
test_builder.cpp:87, which makes scratch under a read-only/tmpon the development machine —the environment defect PR #406 fixes, and a file this change does not touch. CI runners have a
writable
/tmp, so CI is the honest gate here.Fixes #399