fsck should accept stash objects by macneale4 · Pull Request #11298 · dolthub/dolt

macneale4 · 2026-07-13T21:17:23Z

dolt fsck fails when a repository contains a stash because the stash ref (StashRefType) is treated as a commit-pointing ref, so its StashList (SLST) object is validated as a commit (DCMT). This test asserts fsck passes cleanly with a stash present and that the stash survives the check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…11292) fsck treated a stash ref (StashRefType) as a commit-pointing ref, so the StashList (SLST) address it points to was fed into the commit DAG walk and parsed as a commit (DCMT), failing with "incorrect file ID: expected DCMT, got SLST" for any repo containing a stash. A stash ref points to a StashList whose entries each reference the commit the stash was based on (a commit) and the stashed working root (a root value). Resolve the stash list into those objects: feed the head commit into the commit DAG walk (as tags already do) and validate the stashed working root as a tree root. The working root is only reachable through the stash, so it would otherwise never be walked. Generalize Errs.CmtAppendF into refAppendF(kind, ownerHash, ...) so tree errors under a stash root are labeled "::stash:" rather than "::commit:"; CmtAppendF delegates with kind "commit" and its output is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

coffeegoddd · 2026-07-13T21:41:13Z

@macneale4 DOLT

read_tests	from_latency	to_latency	percent_change
covering_index_scan	2.3	2.3	0.0
groupby_scan	147.61	147.61	0.0
index_join	1.96	1.96	0.0
index_join_scan	1.34	1.34	0.0
index_scan	223.34	223.34	0.0
oltp_point_select	0.26	0.26	0.0
oltp_read_only	5.09	5.09	0.0
select_random_points	0.52	0.52	0.0
select_random_ranges	0.64	0.65	1.56
table_scan	200.47	200.47	0.0
types_table_scan	467.3	475.79	1.82

write_tests	from_latency	to_latency
oltp_delete_insert	6.21	6.21
oltp_insert	3.13	3.13
oltp_read_write	11.24	11.24
oltp_update_index	3.3	3.3
oltp_update_non_index	3.02	3.02
oltp_write_only	6.32	6.32
types_delete_insert	6.79	6.79

coffeegoddd · 2026-07-13T21:48:23Z

@macneale4 DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`52bccb6`	ok	5937471

version	total_tests
`52bccb6`	5937471

correctness_percentage
100.0

coffeegoddd · 2026-07-13T21:52:14Z

@macneale4 DOLT

test_name	from_latency_p95	to_latency_p95	percent_change
tpcc-scale-factor-1	45.79	45.79	0.0

test_name	from_server_name	from_server_version	from_tps	to_server_name	to_server_version	to_tps	percent_change
tpcc-scale-factor-1	dolt	`7e268bf`	53.1	dolt	`3b32142`	51.77	-2.5

0x687931 · 2026-07-13T22:50:14Z

PR #11293 was a sound minimal fix and should remain closed as superseded.
PR #11298 has the better overall architecture because it validates the objects referenced by a stash instead of simply excluding stash references from the commit walk.

However, before #11298 is merged, I think two integrity-handling issues should be addressed:

A damaged stash list can currently cause dolt fsck to panic. The helper used to read the stash list ignores some iteration errors and can then dereference an incomplete entry. Since fsck exists to inspect potentially corrupted repositories, this should be reported as an integrity error rather than crashing the process.
The stashed working-root address is walked without first confirming that it actually points to a root-value object. A damaged stash could point to another valid object (such as a commit or schema) and potentially pass validation. The object type should be checked before traversing it.

Also on reporting, the summary now says that its reachability count includes stashes, but the stash-list and individual stash objects themselves are not added to that count.

It would be worth adding corrupted-stash cases for the two issues above, because the current happy-path test does not exercise the new integrity-validation behavior.

macneale4 · 2026-07-13T23:09:49Z

Thanks for your input @0x687931

re 1: fair point

re 2: this is way beyond what fsck can do. All addresses in the data schema could conceivably point at incorrect object types, but the application code is responsible to ensure that doesn't happen. FSCK would need to be aware of every object relation type to verify all of them, and that's never been in scope. To have a valid address to randomly be corrupted into a different valid address is very unlikely. It's far more likely that such a situation would arise as the result of a bug in the application code.

Adding more tests is more work than it's worth. creating corrupted databases and actually a fair amount of work. We are actually far better served with real examples of corruption from the wild.

resolveStashRefs read the stash list via datas.GetHashListFromStashList, whose getStashListOrdered swallows the address-map iteration error and can leave nil entries; a damaged stash list (e.g. a non-numeric address-map key) then nil-dereferences and crashes the process. fsck exists to inspect possibly-corrupt repositories, so this must be reported, not fatal. Walk the stash list address map directly instead, checking the IterAll error, and read each stash through resolveStash which reports bad file IDs, missing objects, and malformed address fields as integrity errors while continuing the scan. Adds a unit test that resolves a corrupt stash list (the input that panics the old helper) and asserts it is handled without panicking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

0x687931 · 2026-07-13T23:11:13Z

You’re right 😜 I was narrowly looking at my problem

…

On Tue, 14 Jul 2026 at 9:10 am, Neil Macneale IV ***@***.***> wrote: *macneale4* left a comment (dolthub/dolt#11298) <#11298 (comment)> Thanks for your input @0x687931 <https://github.com/0x687931> re 1: fair point re 2: this is way beyond what fsck can do. All addresses in the data schema could conceivably point at incorrect object types, but the application code is responsible to ensure that doesn't happen. FSCK would need to be aware of every object relation type to verify all of them, and that's never been in scope. To have a valid address to randomly be corrupted into a different valid address is very unlikely. It's far more likely that such a situation would arise as the result of a bug in the application code. Adding more tests is more work than it's worth. creating corrupted databases and actually a fair amount of work. We are actually far better served with real examples of corruption from the wild. — Reply to this email directly, view it on GitHub <#11298?email_source=notifications&email_token=AGLTHGNQZS4LVVE7ZM37SHL5EVT5HA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJWGM3DQMZZGM22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4963683935>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGLTHGKZZZHNGASG33FKJV35EVT5HAVCNFSNUABFKJSXA33TNF2G64TZHMYTSOBWHAZTKNZUHNEXG43VMU5TIOBXG43DONZZG442C5QC> . You are receiving this because you were mentioned.Message ID: ***@***.***>

coffeegoddd · 2026-07-13T23:43:41Z

@macneale4 DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`57a7b53`	ok	5937471

version	total_tests
`57a7b53`	5937471

correctness_percentage
100.0

elianddb

lgtm, but one comment

elianddb · 2026-07-13T23:49:35Z

+// Because fsck must tolerate corrupt repositories, the stash list address map is walked directly rather than via the
+// datas stash helpers, which assume well-formed entries and can panic on damaged data. Any problem reading the list or
+// its entries is appended to errs and skipped rather than aborting the scan.
+func resolveStashRefs(ctx context.Context, cs chunks.ChunkStore, ns tree.NodeStore, stashListAddr hash.Hash, refName string, errs *Errs) (headCommits []hash.Hash, stashRoots []hash.Hash) {


Isn't this similar to the checks getRawReferencesFromStoreRoot does, is there a possibility to deduplicate?

The store-root reader and the stash-list reader both parsed an embedded serialized AddressMap the same way: decode the node, verify the ADRM file ID, and build the prolly.AddressMap. Extract that into addressMapFromBytes and call it from both getRawReferencesFromStoreRoot and resolveStashRefs. Addresses review feedback on PR #11298. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

coffeegoddd · 2026-07-14T00:28:56Z

@macneale4 DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`3b32142`	ok	5937471

version	total_tests
`3b32142`	5937471

correctness_percentage
100.0

github-actions · 2026-07-14T03:06:08Z

@coffeegoddd DOLT

name	add_cnt	delete_cnt	update_cnt	latency
adds_only	60000	0	0	0.6
adds_updates_deletes	60000	60000	60000	2.87
deletes_only	0	60000	0	1.47
updates_only	0	0	60000	1.71

github-actions · 2026-07-14T03:07:28Z

@coffeegoddd DOLT

test_name	detail	row_cnt	sorted	mysql_time	sql_mult	cli_mult
batching	LOAD DATA	10000	1	0.05	2
batching	batch sql	10000	1	0.06	2
batching	by line sql	10000	1	0.06	2
blob	1 blob	200000	1	0.86	2.79	2.63
blob	2 blobs	200000	1	0.86	2.76	2.6
blob	no blob	200000	1	0.86	2.77	2.64
col type	datetime	200000	1	0.84	2.51	2.37
col type	varchar	200000	1	0.66	3.91	3.64
config width	2 cols	200000	1	0.76	2.67	2.46
config width	32 cols	200000	1	1.71	3.36	2.95
config width	8 cols	200000	1	0.93	2.9	2.76
pk type	float	200000	1	0.83	2.52	2.28
pk type	int	200000	1	0.77	2.64	2.42
pk type	varchar	200000	1	1.39	1.8	1.6
row count	1.6mm	1600000	1	5.71	2.92	2.65
row count	400k	400000	1	1.46	2.81	2.53
row count	800k	800000	1	2.81	2.94	2.65
secondary index	four index	200000	1	3.44	1.32	1.13
secondary index	no secondary	200000	1	0.86	2.79	2.62
secondary index	one index	200000	1	1.08	2.65	2.27
secondary index	two index	200000	1	1.87	1.83	1.97
sorting	shuffled 1mm	1000000	0	5.13	2.91	2.77
sorting	sorted 1mm	1000000	1	5.09	2.93	2.84

github-actions · 2026-07-14T03:13:32Z

@coffeegoddd DOLT

name	detail	mean_mult
dolt_blame_basic	system table	1.26
dolt_blame_commit_filter	system table	1.18
dolt_commit_ancestors_commit_filter	system table	0.69
dolt_commits_commit_filter	system table	1.1
dolt_diff_log_join_from_commit	system table	3.13
dolt_diff_log_join_to_commit	system table	3.17
dolt_diff_table_from_commit_filter	system table	1.18
dolt_diff_table_to_commit_filter	system table	1.22
dolt_diffs_commit_filter	system table	1.14
dolt_history_commit_filter	system table	1.48
dolt_log_commit_filter	system table	1.21

macneale4 and others added 2 commits July 13, 2026 20:00

macneale4 mentioned this pull request Jul 13, 2026

Fix fsck handling of stash refs #11293

Closed

coffeegoddd added the performance_approved label Jul 13, 2026

coffeegoddd added the correctness_approved label Jul 13, 2026

macneale4 requested a review from elianddb July 13, 2026 23:21

elianddb approved these changes Jul 13, 2026

View reviewed changes

macneale4 merged commit 6b2372c into main Jul 14, 2026
26 checks passed

macneale4 deleted the macneale4-claude/fsck-stash branch July 14, 2026 00:46

BrewTestBot mentioned this pull request Jul 15, 2026

dolt 2.1.11 Homebrew/homebrew-core#293230

Merged

Uh oh!

Uh oh!

Conversation

macneale4 commented Jul 13, 2026

Uh oh!

coffeegoddd commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coffeegoddd commented Jul 13, 2026

Uh oh!

coffeegoddd commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x687931 commented Jul 13, 2026

Uh oh!

macneale4 commented Jul 13, 2026

Uh oh!

0x687931 commented Jul 13, 2026 via email

Uh oh!

coffeegoddd commented Jul 13, 2026

Uh oh!

elianddb left a comment

Choose a reason for hiding this comment

Uh oh!

elianddb Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

coffeegoddd commented Jul 14, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jul 14, 2026

Uh oh!

github-actions Bot commented Jul 14, 2026

Uh oh!

github-actions Bot commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coffeegoddd commented Jul 13, 2026 •

edited

Loading

coffeegoddd commented Jul 13, 2026 •

edited

Loading