Skip to content

fix(sync): stop pinning file mtime to Notion last_edited_time#75

Merged
ran-codes merged 3 commits into
mainfrom
feat/mtime-desync-file-notion
May 5, 2026
Merged

fix(sync): stop pinning file mtime to Notion last_edited_time#75
ran-codes merged 3 commits into
mainfrom
feat/mtime-desync-file-notion

Conversation

@ran-codes
Copy link
Copy Markdown
Member

Context

  • Production silent-loss observed in salurbal.org PR #580: a typo fix issued via the Notion API + notion-sync refresh --ids left the corrected content on disk, but git status reported the file clean and the commit captured the pre-fix state.
  • Root cause: FreezePage called os.Chtimes after every write to set the file's mtime backwards to Notion's last_edited_time. Git's stat-cache skips re-hashing when (size, mtime, inode) are unchanged from the cached entry, so a same-length rewrite where Notion's timestamp also did not advance is invisible to git status.
  • Both trigger conditions hold for routine same-length API edits (typo corrections, equal-length renames, certain property toggles whose last_edited_time does not bump) — not a rare corner.
  • The Notion timestamp is already preserved in YAML frontmatter (notion-last-edited), which is git-tracked and human-readable. Filesystem mtime was a redundant copy, and no internal caller reads ModTime() for any decision.

Changes

  • Drop the os.Chtimes mtime-pin block in internal/sync/page.go so the OS sets mtime to wall-clock-now at write time. Git's stat-cache then invalidates correctly on every rewrite.
  • Add TestFreezePage_DoesNotPinMtimeToNotionTimestamp in internal/sync/page_test.go — freezes a page with a 2020 last_edited_time and asserts the resulting file's mtime is not in the past. Encodes the contract so the pin cannot silently regress.
  • notion-last-edited frontmatter remains the authoritative source of truth for "when did Notion last touch this entry"; behavior of skip-if-unchanged checks (which read frontmatter, not mtime) is unaffected.
  • Full test suite (go test ./...) passes.

🤖 Generated with Claude Code

ran-codes and others added 2 commits May 5, 2026 11:21
After every page write, FreezePage called os.Chtimes to roll the file's
mtime back to Notion's last_edited_time. This defeats git's stat-cache:
when a refresh produces a same-length rewrite AND Notion's timestamp did
not advance, (size, mtime, inode) are unchanged from the cached entry,
so `git status` reports the file clean despite the on-disk content
having changed. Workflows that run `notion-sync refresh && git commit`
silently lose the update.

Both trigger conditions hold for routine same-length API edits (typo
fixes, equal-length renames, certain property toggles whose
last_edited_time does not bump). Observed in production on a typo
correction that did not surface in `git status`.

The authoritative timestamp is preserved in the `notion-last-edited`
YAML frontmatter field, which is git-tracked and human-readable; the
filesystem mtime was a redundant copy that no internal caller reads.
Letting the OS set mtime to wall-clock-now restores stat-cache
correctness with no loss of provenance.

Adds a regression test asserting mtime is not pinned to a past Notion
timestamp after FreezePage.

Co-Authored-By: Claude <noreply@anthropic.com>
@ran-codes
Copy link
Copy Markdown
Member Author

Issue Evaluation

Verdict: real bug, valuable fix, trivial change.

Verified. internal/sync/page.go:135-138 calls os.Chtimes(filePath, lastEdited, lastEdited) after every write, pinning mtime to Notion's last_edited_time. Git's stat-cache skips re-hashing when (size, mtime, inode) match the cached entry, so a same-length rewrite where Notion's timestamp also did not advance is invisible to git status — a silent-loss vector for any "API edit → refresh → commit" workflow. Both trigger conditions hold for routine same-length API edits (typo fixes, equal-length renames, certain property toggles whose last_edited_time does not bump).

No internal consumer of mtime. The skip-if-unchanged check at page.go:72 reads notion-last-edited from YAML frontmatter, not filesystem mtime. push.go likewise reads the frontmatter field. Nothing in the codebase calls .ModTime(). Removing the Chtimes call breaks no decision logic.

Feasibility: trivial. Source change is 4 lines deleted; time import stays (still used elsewhere in the file). No flags, no metadata schema impact, no callers to update. Regression test added to encode the contract so the pin can't silently come back.

Caveats worth flagging: anyone downstream depending on mtime ordering (rare) gets a behavior change. Reproducibility is preserved — notion-last-edited frontmatter remains the authoritative source of truth.

@ran-codes
Copy link
Copy Markdown
Member Author

TDD Cycle Summary

Cycle: RED → GREEN → full suite.

RED — failing regression test

Added TestFreezePage_DoesNotPinMtimeToNotionTimestamp in internal/sync/page_test.go. Encodes the contract: after FreezePage writes a file, its mtime must reflect actual write time, not Notion's last_edited_time.

Conditions under test:

  • Page with last_edited_time = "2020-01-01T00:00:00Z" (deliberately ancient)
  • before = time.Now() - 2s captured pre-call (margin for FAT/NTFS mtime quantization)
  • Assertion: info.ModTime() >= before after FreezePage

Result: failed as expected

file mtime = 2019-12-31T19:00:00-05:00 is before test start 2026-05-05T12:55:48… —
looks pinned to Notion's last_edited_time (2020-01-01T00:00:00Z); this defeats
git's stat-cache

GREEN — minimal fix

Deleted the os.Chtimes block (4 lines) at internal/sync/page.go:135-138. Let the OS set mtime to wall-clock-now naturally.

Result: TestFreezePage_DoesNotPinMtimeToNotionTimestamp passes.

Full suite

go test ./... — all packages green (cmd, internal/clean, internal/config, internal/frontmatter, internal/markdown, internal/notion, internal/sync, internal/util). go build ./... clean.

Code changes

File Δ
internal/sync/page.go −4 (drop os.Chtimes block)
internal/sync/page_test.go +41 (regression test + time import)

Net: −4 source / +41 test.

The test-single-datasource-db and test-double-datasource-db skills had
explicit pass criteria that "file mtime matches notion-last-edited
within 1s." That criterion encoded the very behavior PR #75 removes —
pinning mtime backwards defeats git's stat-cache and causes silent loss
on same-length rewrites.

Inverts both checks to the new contract: mtime must NOT be within 1s of
notion-last-edited. Provides defense in depth alongside the unit-level
TestFreezePage_DoesNotPinMtimeToNotionTimestamp regression guard.

Updates testing-README pyramid bullet from "mtime" to "no-pin" to match.

Co-Authored-By: Claude <noreply@anthropic.com>
@ran-codes
Copy link
Copy Markdown
Member Author

Code Review (Pass 1)

Note

Verdict: Approve
Correct, minimal fix; root-cause analysis is sound and the regression test encodes the contract.

Suggestions

None — author confirmed remaining items are sharpening, not no-brainers.

Notes

  • Root cause(size, mtime, inode) stat-cache invariant correctly identified; pinning to a non-advancing Notion timestamp on a same-length rewrite makes git blind to the change.
  • No internal dependency on mtimegrep ModTime across the repo confirms no caller reads os.Stat().ModTime() for any decision; mtime was a redundant copy of notion-last-edited frontmatter.
  • time import retained correctly — still used at internal/sync/page.go:196,249 after the os.Chtimes deletion; no dangling import.
  • E2E criteria inversion is the right move — the previous "mtime within 1s of notion-last-edited" pass criterion in both SKILL.md files literally encoded the bug; inverting them turns the e2e tests into a defense-in-depth regression guard alongside the unit test.
  • Unit test scope — covers the create path (fresh t.TempDir()), which matches the single unconditional write site in FreezePage. Sufficient for the bug being guarded.

Review feedback assisted by the critical-code-reviewer skill.

@ran-codes
Copy link
Copy Markdown
Member Author

E2E Verification: /test-double-datasource-db

Ran the inverted Step 11 contract end-to-end against the real Notion API on the double-data-source test database (c9aa5ab2-b470-429c-ba9c-86c853782bb2, 14 pages across Projects/ + Clients/ subfolders).

Step 11 — Verify file mtime is not pinned to Notion's last_edited_time

For each synced .md file, compared filesystem mtime (via stat -c %Y) against the notion-last-edited value in YAML frontmatter, after a force refresh rewrote every file.

Result:

Files passing (mtime NOT within 1s of notion-last-edited): 14
Files failing (mtime pinned within 1s):                     0

Sample (Delta Corp, the page edited via Notion MCP in Step 8 — strongest case for a flake since both timestamps were freshly set in this run):

mtime:              2026-05-05 13:16:56.539612700 -0400  (= 17:16:56 UTC)
notion-last-edited: 2026-05-05T17:16:00.000Z            (= 17:16:00 UTC)
diff:               56 seconds — well outside 1s tolerance

The remaining 13 files (untouched by the MCP edit) have notion-last-edited values from days/weeks ago and mtimes from the just-completed force refresh — diff measured in days, not seconds. No false-positive flake on the recently-edited file.

Pre-fix counterfactual

Under the old os.Chtimes behavior, every file's mtime would equal notion-last-edited exactly (zero diff), failing the inverted check for all 14 files. The 56-second gap on Delta Corp is direct evidence the pin is gone and mtime now reflects actual write time — restoring git stat-cache correctness.

Surrounding steps (full run)

All 13 steps passed. Notion-state revert (Step 12) confirmed clean — Revenue restored to 125000, content marker removed, test database left in original state.

ALL TESTS PASSED

@ran-codes ran-codes merged commit d3e6390 into main May 5, 2026
1 check passed
@ran-codes ran-codes deleted the feat/mtime-desync-file-notion branch May 5, 2026 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant