Delete the unreachable mtime stamping and say why it is gone - #40
Merged
Conversation
My own 62a30b8 removed the last call that stamped an origin mtime onto a materialized file. It left the machinery behind, and it left a doc comment arguing that stamping matters. Both are now wrong. write_atomic_with_mtime had exactly one caller, write_atomic, which always passed None. So the parameter was unreachable, and the comment explained the value of behaviour the code no longer had. That is the same defect as the replay-buffer text: a written claim that does not match the code, in a file I edited. The two functions collapse into one. write_atomic now carries the real reason: stamping made mtime a cross-node value, two contending entries of equal size could collide on size plus mtime, and the scan cache then reported content the file did not hold, which is the permanent three-node divergence. The comment also records the consequence, so the next reader does not have to rediscover it: fabric replicates no mtime at all today, so a consumer must not read a replica's mtime as an activity signal. Issue 27 is exactly that mistake. set_file_mtime is kept and marked test-only rather than deleted. It is not dead: the test that reconstructs the size-plus-mtime collision needs it to build the collision on purpose. Deleting it would have removed the proof that the divergence mechanism is understood. No behaviour change. 195 lib tests green.
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.
Cleanup only, no behaviour change. Approved by cos as the part of #27 that needs no contract decision.
What was wrong
My own
62a30b8removed the last call that stamped an origin mtime onto a materialized file. It left the machinery behind, and it left a doc comment arguing that stamping matters.write_atomic_with_mtimehad exactly one caller —write_atomic, which always passedNone. The parameter was unreachable, and the comment explained the value of behaviour the code no longer had.Same defect as the replay-buffer text in #39: a written claim that does not match the code, in a file I edited. Second one tonight, both found by reading code rather than prose.
What this does
The two functions collapse into one.
write_atomicnow carries the real reason stamping is gone: it made mtime a cross-node value, two contending entries of equal size could collide on size + mtime, and the scan cache then reported content the file did not hold — the permanent three-node divergence.The comment also records the consequence, so the next reader does not rediscover it the hard way: fabric replicates no mtime at all today, so a consumer must not read a replica's mtime as an activity signal. #27 is exactly that mistake.
What I did not delete
set_file_mtimeis kept and marked#[cfg(test)]rather than removed. It is not dead — the test that reconstructs the size-plus-mtime collision needs it to build that collision on purpose. Deleting it would have removed the proof that the divergence mechanism is understood. Calling it "dead code" and dropping it would have been the easy wrong answer.Scope
This does not prejudge the #27 contract. cos is taking that to st2's owner and to Nathan, with the recommendation to put the heartbeat in the file bytes and declare metadata-only changes non-replicating.
Tests
195 lib tests green. Changed file passes rustfmt.