Skip to content

Compare log-file identities as BigInt, so Windows file IDs past 2^53 stop matching neighbouring files - #3036

Merged
kriszyp merged 4 commits into
mainfrom
fix/exact-log-file-identity
Oct 6, 2026
Merged

kriszyp merged 4 commits into
mainfrom
fix/exact-log-file-identity

Conversation

@kriszyp

@kriszyp kriszyp commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

⊙ Problem

Unit Test (Windows, Node.js v24) went red on main in 3 of the last 9 completed runs (ad6feb1, 6c6eb2d, ce32b18), every time at logGenerationCoordinator.test.js:172: expected a foreign generation to leave the descriptor alone (2 !== 1). Windows reports the 64-bit NTFS file ID as st_ino. Read as a Number, any ID past 2^53 (an MFT record reused 32+ times) rounds, so the test's "foreign" held.ino + 1 equals held.ino. Not caused by #2648, the merge at the red head: the test dates from 2026-09-09 and fails whenever the runner's file lands on a high ID.

The same rounding applies in production. Every log-generation identity check read Number stats, so on such a volume two neighbouring files compare equal:

  • The retention-time stale sweep can keep a descriptor on an archived generation and answer "released", so the archive is destroyed while that descriptor still appends to it.
  • The write-path guard keeps appending to a file another thread rotated away.
  • The interval clock misses another writer's rotation and rotates the young replacement.

❓ Your call: Fix the production comparison, not just the test? Chosen: yes. The test's assumption (ino + 1 names another file) is right; the Number representation breaks it, and a test-only change (held.ino * 2) would turn CI green while leaving the stale sweep fail-open on Windows hosts. Reverting to the test-only change is a one-line edit.

💡 Solution

Every (dev, ino) that log rotation compares is now read with { bigint: true }, which Node returns exactly for 64-bit IDs. That covers all 8 identity reads in 4 modules: the descriptor-open fstat in harper_logger.ts; in logRotation.ts, rotateLogFileSync's active and archive stats and the write-path checkpoint; the stale sweep in logGenerationCoordinator.ts; and the 3 logRotator.ts stats (startup, size tick, interval clock).

⚠️ Look hardest: the stale sweep's live stat. It is the backstop that must prove every descriptor released before retention destroys an archive, and the one site where the old rounding failed open.

A FileIdentity { ino: bigint; dev: bigint } type now sits on the sink identity, the announced generation, publishArchivedGeneration and the guard's getLogIdentity. A Number producer is a compile error (dropping { bigint: true } from the fstat gives TS2322), not a silent bigint === number mismatch, which would fail open in releaseLocally.

Edges that BigInt stats change:

  • holdsGeneration's zero-inode check becomes a falsy check, because 0n === 0 is false.
  • birthtimeMs is a BigInt on BigInt stats, so it is converted with Number() before Math.min. Without that, Math.min throws inside a swallowed catch and the initial interval clock is lost.
  • Generations cross threads over postMessage, which structured-clones BigInt (verified with a MessageChannel round trip). Nothing JSON-serializes a generation.
  • Cost: statSync 1070 → 1242 ns and fstatSync 727 → 833 ns (Node 26.2, 200k iterations). That is paid once per maxBytes / 16 bytes written and once per descriptor open.

Size stats and the directory device check stay Number: they never compare file identity. The invariant is recorded in utility/DESIGN.md and indexed in the root DESIGN.md.

⚖️ Alternatives

  • Test-only (held.ino * 2): CI goes green and the production fail-open remains.
  • Treat ino > Number.MAX_SAFE_INTEGER as unknown, like ino 0: identity checks become no-ops for a large share of Windows files, and every stale sweep closes every descriptor.
  • Generation tokens instead of filesystem identity: a descriptor another thread opened by pathname cannot know which generation it got.
  • Planning review: Framing-Verdict: chosen-approach-sound (Codex).

❓ Your call: BigInt stats at every identity read, versus Number stats with a BigInt re-stat only when the Number values match. Chosen: uniform BigInt, about one extra allocation per 4 MB of log output. Easy to switch.

❓ Your call: resources/blob.ts:722 has the same Number-ino comparison (blob swap detection after an in-place repair). It is left for a follow-up so this PR stays scoped to the red test and logging.

✅ Verification

  • Reproduced on Linux at base ad6feb1. A mocha --require hook that reports every ino past 2^53 makes logGenerationCoordinator.test.js:172 fail with the CI signature (2 !== 1). Under a hook that collapses every file to one Number, 4 logging tests fail on base. With the fix, 199 pass under that hook.
  • New regression test. logFileIdentity.test.js runs fixtures/highFileIdLogging.cjs in a child process, with every file given a distinct ID past 2^53. It covers the stale sweep, the write-path guard and the interval clock against a real logger. Each scenario fails on base with its own assertion and passes with the fix, and 8 of 8 concurrent runs pass.
  • Changed tests. The announced-generation test in logRotationGuard.test.js now announces the identity from the real rotateLogFileSync. With only the fstat change reverted, it fails (the marker lands in the archive). The coordinator tests use BigInt stats, so + 1n and + 1000n stay exact.
  • Suites. test:unit:main: 6728 passing, 0 failing. test:unit:resources: 4007 passing. Logging suite: 199 passing. integrationTests/server/log-rotation-write-path.test.ts: 2 of 2, with real HTTP workers, the real postMessage transport, and every request marker exactly once.
  • Checks. Build (tsc), oxlint on the changed directories, prettier and check:design-docs are all clean.

❓ Your call: The high-ID emulation patches the synchronous fs.statSync/fstatSync/lstatSync in a child process, rather than adding an injectable stat seam to production code. That keeps production untouched, and it covers every identity read because all of them are synchronous. An async identity read added later would bypass it.

❓ Your call: The interval scenario fakes Date.now by hand (the house rule bars new sinon use) and asserts only after the log has been statted three times on the advanced clock. A tick stats it once, or twice when it rotates, and ticks never overlap, so a whole tick has run; that ties the test to the tick's stat count.

  • Not run locally: the full test:integration:all and Windows; this PR's CI runs both. The TypeStrip mocha harness fails at init on main too (json/systemSchema.json needs an import attribute); the changed modules themselves load under --conditions=typestrip.

🤖 Generated by Claude Opus 5.5 (Claude Code); posted via @kriszyp.

🤖 Generated with Claude Code

Related PRs: #372 overlaps (if its status view serializes a sink's descriptor identity, that is now a BigInt and needs a string form), #2939 overlaps (also edits harper_logger.ts and the end of utility/DESIGN.md; expect a text conflict only), #562 independent, #1835 independent, #2155 independent, #2170 independent, #2446 independent, #2649 independent, #2752 independent, #2763 independent, #2775 independent, #2901 independent, #2906 independent, #2930 independent, #2986 independent, #2990 independent, #2994 independent, #3021 independent, #3030 independent, #3035 independent, #2981 independent
Complexity: medium

Origin — the dispatch brief this PR was written from

harper main is red: Unit Test at ad6feb1

main on HarperFast/harper went from green to red. Failing workflows: Unit Test at ad6feb1.

Find the merge that broke it and get main green. Walk that workflow's recent push runs on main oldest-to-newest to find the FIRST red head SHA and the PR it came from, then pull the failing test names from that run and from the current head. A failure on every matrix leg (Node version, Bun, uWS, Windows) is a regression; one leg is usually a flake.

Then either fix it or, when the culprit is a single merge and the fix is not obvious, open a revert and say so — main being green is worth more than the change being preserved. Record the fingerprint on the initiatives/ci-health board doc either way (prose row beginning with the test path, never a bare ref), and add your PR to initiatives/testing-and-deploy > "CI & build infrastructure" as a bare-ref line so it renders as a card.

Dispatch: task main-red-kriszyp_harper_ad6feb127_4d7eefda · queued by automation · ran by claude/opus/xhigh · worker kzyp-xps-1

Review-Coverage: authored=claude; ran=gemini; adjudicated=domain; blocked=codex(out-of-budget),claude(fallback)(out-of-budget); declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=4; full=1 @ 8fcab59

Review-Attention: read ~5m (decisions: stat-count-tick-signal, bigint-on-every-stat, fs-monkeypatch-emulation, scope-logging-only; raised: degraded review) @ 8fcab59

kriszyp and others added 4 commits October 5, 2026 22:47
…stop matching neighbouring files

Windows reports a 64-bit NTFS file ID as st_ino. Read as a Number, any ID past
2^53 (an MFT record reused 32+ times) rounds, and two neighbouring files compare
equal. Every log-generation check read Number stats, so on such a volume the
retention-time stale sweep could keep a descriptor on an archived generation and
answer "released", the write-path guard kept appending to a file another isolate
rotated away, and the interval clock missed another writer's rotation. The
coordinator test's `held.ino + 1` rounded back to `held.ino`, which is the
intermittent Windows unit failure on main (logGenerationCoordinator.test.js:172).

Read every identity with { bigint: true } (descriptor open, rotation's
active/archive stat, write-path checkpoint, stale sweep, rotator) and type the
sink identity and announced generation as FileIdentity { ino: bigint; dev:
bigint }, so a Number producer fails to compile rather than silently comparing
unequal. A child-process test emulates a high-ID volume and covers the sweep,
the guard and the interval clock; each scenario fails on the previous code.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: main-red-kriszyp_harper_ad6feb127_4d7eefda
…ng margins, and type the published generation

The fixture's `ino % 2^17` projection could merge two real files; a per-process
index cannot. The interval scenario now ends a fixed 150ms past the first
generation's interval, measured from its own start, leaving more than 450ms of
scheduling slack on either side. publishArchivedGeneration takes the generation
rotateLogFileSync returns, so its BigInt identity is checked there too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: main-red-kriszyp_harper_ad6feb127_4d7eefda
…uard's options

Only Date is faked, so the audit ticks stay real but time moves only when the
scenario advances it: a late tick can postpone a rotation, never cause one, so
the scenario cannot fail on a slow runner. createRotationGuard's options are
typed, so getLogIdentity carries FileIdentity too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: main-red-kriszyp_harper_ad6feb127_4d7eefda
…whole audit tick has run

AGENTS.md rules out new sinon uses; the fixture is its own process, so it
replaces Date.now directly and restores it. The fixed sleep before the
assertion is replaced by waiting until the log has been statted three times
after the clock advance: a tick stats it once, or twice when it rotates, and
ticks never overlap, so the first tick on the advanced clock has finished.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: main-red-kriszyp_harper_ad6feb127_4d7eefda

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the log rotation and coordination mechanism to compare log-file identities using BigInt instead of standard Number types. This prevents 64-bit file ID collisions on Windows filesystems (such as NTFS) where file IDs can exceed 2^53 and round to identical values when represented as standard JavaScript numbers. The changes enforce the use of { bigint: true } across all fs.statSync and fs.fstatSync calls involved in log identity checks, introduce a new FileIdentity type, and add comprehensive integration tests to emulate and verify correct behavior on volumes with high file IDs. No review comments were provided, and I have no feedback to provide on these changes.

@kriszyp
kriszyp marked this pull request as ready for review October 6, 2026 12:46
@kriszyp
kriszyp merged commit 20b3349 into main Oct 6, 2026
51 checks passed
@kriszyp
kriszyp deleted the fix/exact-log-file-identity branch October 6, 2026 12:47
kriszyp added a commit that referenced this pull request Oct 6, 2026
Brings the branch onto current main (including #3036) so the harper-pro pin of this branch no longer reverts main's core history.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QW9pttHCFmEN8mPjAxejn
Dispatch-Task: pr-maint-b389c5b8f1eb01dd12a0926e3c74fcbd
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