Skip to content

Preserve integral precision when parsing decimal Duration inputs - #6945

Open
fubhy wants to merge 1 commit into
mainfrom
audit/repro-core-duration-parse-precision
Open

Preserve integral precision when parsing decimal Duration inputs#6945
fubhy wants to merge 1 commit into
mainfrom
audit/repro-core-duration-parse-precision

Conversation

@fubhy

@fubhy fubhy commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

Fractional nano and micro strings above Number.MAX_SAFE_INTEGER can change integral digits before nanosecond rounding.

Important

This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.

Decimal nano parsing corrupts integral precision

Module: Duration
Audit ID: core-a-f-duration-decimal-precision
Severity / confidence: medium / high

What happens

Fractional nano and micro strings above Number.MAX_SAFE_INTEGER can change integral digits before nanosecond rounding.

Why it happens

parseNanos converts the complete decimal and scale through Number before rounding to bigint, losing precision that remains available in the original string.

Expected behavior

Decimal duration strings are parsed into the nanosecond-backed representation and rounded to integer nanoseconds without corrupting the integral component.

Relevant implementation

These links and excerpts are pinned to audit base c9b56ab507f224426ee8388dc450da447ec4715f.

View problematic code at packages/effect/src/Duration.ts:36-42
const roundTiesAwayFromZero = (input: number): bigint =>
  BigInt(input < 0 ? Math.ceil(input - 0.5) : Math.floor(input + 0.5))

const roundMillisToNanos = (millis: number): bigint => roundTiesAwayFromZero(millis * 1_000_000)

const parseNanos = (input: string, scale: bigint): bigint =>
  input.includes(".") ? roundTiesAwayFromZero(Number(input) * Number(scale)) : BigInt(input) * scale

View exact lines on GitHub

View problematic code at packages/effect/src/Duration.ts:241-249
      const match = DURATION_REGEXP.exec(input)
      if (!match) break
      const [_, valueStr, unit] = match
      if (unit === "nano" || unit === "nanos") {
        return nanos(parseNanos(valueStr, bigint1))
      }
      if (unit === "micro" || unit === "micros") {
        return nanos(parseNanos(valueStr, bigint1e3))
      }

View exact lines on GitHub

Reproduction

pnpm test --run packages/effect/test/DurationParsePrecision.test.ts

Observed failure: A direct Node 24 source probe parsed 9007199254740993.1 nanos as 9007199254740994n instead of 9007199254740993n.

Implementation handoff

The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.

  1. Start with the pinned implementation excerpts and the Why it happens analysis above.
  2. Change the implementation so it satisfies the stated Expected behavior; do not weaken or remove the reproduction assertions.
  3. Run the focused reproduction command(s) and confirm the observed failures become passing tests:
pnpm test --run packages/effect/test/DurationParsePrecision.test.ts
  1. Run the affected package's existing tests, then the repository lint and type checks before requesting review.

Audit provenance

  • Audit base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Reproduction base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Findings: core-a-f-duration-decimal-precision
  • Initial patch: focused reproduction tests; implementation fix pending

@fubhy fubhy added the audit Findings originating from the Effect runtime correctness audit label Aug 4, 2026
@github-project-automation github-project-automation Bot moved this to Discussion Ongoing in PR Backlog Aug 4, 2026
@changeset-bot

changeset-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 642d7a9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@effect-slopcop effect-slopcop Bot added 4.0 bug Something isn't working labels Aug 4, 2026

@pullfrog pullfrog 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.

Important

The regression test is a good spec, but the implementation fix still needs to land on this branch before the PR can merge.

Reviewed changes

I reviewed the single regression test added in packages/effect/test/DurationParsePrecision.test.ts and ran it against the current main-based implementation. As expected, the test fails because parseNanos in packages/effect/src/Duration.ts converts the whole decimal string through Number before scaling, which corrupts integral digits above Number.MAX_SAFE_INTEGER.

The reproduction command in the PR body confirmed the failure, and the existing Duration.test.ts suite still passes (no source changes yet).

⚠️ Implementation fix is missing

The diff only adds the failing test; packages/effect/src/Duration.ts:41-42 still uses roundTiesAwayFromZero(Number(input) * Number(scale)) for decimal nano/micro strings. Merging this branch as-is will keep CI red.

Technical details
# Missing `parseNanos` precision fix

## Affected sites
- `packages/effect/src/Duration.ts:36-42``roundTiesAwayFromZero` and `parseNanos`
- `packages/effect/src/Duration.ts:244-249` — nano/micro parsing call sites
- `packages/effect/test/DurationParsePrecision.test.ts:7-10` — correct regression assertion

## Required outcome
- Decimal nano and micro strings must be parsed into bigint nanoseconds without first coercing the full string through `Number`.
- Rounding rules (ties away from zero) should still apply to the final nanosecond value.
- The existing `Duration.test.ts` assertions for `1.5 nanos`, `1.5 micros`, etc., must continue to pass.

## Suggested approach (optional)
Split `valueStr` on `.`, scale the integral part with `BigInt` arithmetic, scale the fractional part (right-padded/truncated to the unit's scale), and add them before rounding. For example, for micros scale `1_000` and fractional string `1`, compute `(BigInt(int) * 1_000n) + BigInt(frac.padEnd(3, "0").slice(0, 3))`, then round. Handle the sign separately so negative inputs round away from zero correctly.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏


describe("Duration decimal parsing", () => {
it("preserves integer precision before rounding nanos", () => {
strictEqual(

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.

This is a correct regression assertion — it failed exactly as described, returning 9007199254740994n instead of the expected 9007199254740993n. Keep it when the parseNanos fix is added.

"decimal nanos should be rounded without first losing integer precision"
)
})
})

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.

Consider adding a micro case as well (e.g., "9007199254740993.1 micros" with the corresponding scaled expectation) since parseNanos is also used for the micro path with a scale of 1_000. Negative tie cases would also tighten the regression suite.

@github-project-automation github-project-automation Bot moved this from Discussion Ongoing to Waiting on Author in PR Backlog Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.0 audit Findings originating from the Effect runtime correctness audit bug Something isn't working

Projects

Status: Waiting on Author

Development

Successfully merging this pull request may close these issues.

1 participant