MetaMask / MetaMask/metamask-extension

MetaMetrics event fragment timeout check mixes milliseconds and seconds

Open Beginner friendly
#42,895 1 comment 0 reactions 0 assignees View on GitHub
external-contributor INVALID-ISSUE-TEMPLATE
Dominant language
TypeScript
Stars
13.2k
Forks
5.6k
Avg merge
2d 5h
Merged PRs (30d)
451

Description

I think the event fragment timeout check in MetaMetrics is using the wrong unit conversion.

Current code in `app/scripts/controllers/metametrics-controller.ts`:

```ts
Date.now() - fragment.lastUpdated / 1000 > fragment.timeout
```

But the fragment contract says:

- `timeout` is in seconds
- `lastUpdated` is `Date.now()`

From `shared/constants/metametrics.ts`:

```ts
/**
* Time in seconds the event should be persisted for.
*/
timeout?: number;

/**
* `Date.now()` when the fragment was last updated.
*/
lastUpdated?: number;
```

And `lastUpdated` is written with `Date.now()` when fragments are created/updated.

Because `/` has higher precedence than `-`, the current expression is evaluated as:

```ts
Date.now() - (fragment.lastUpdated / 1000)
```

not:

```ts
(Date.now() - fragment.lastUpdated) / 1000
```

So any fragment with both `timeout` and `lastUpdated` would become eligible for abandonment much earlier than intended.

I think the check should be one of these instead:

```ts
Date.now() - fragment.lastUpdated > fragment.timeout * 1000
```

or

```ts
(Date.now() - fragment.lastUpdated) / 1000 > fragment.timeout
```

A bit of history in case it helps:

- introduced with event fragments in commit `b820ef131b` (`Implement event fragments`, PR #12251)
- carried forward through later refactors/migrations including `29c2b136b8`, `317d2ef12e`, and `6ae02e8e8a`

I didn’t find a current in-tree caller that sets `timeout`, so this may be latent today rather than something widely hit in production, but the timeout path itself looks incorrect as implemented.

Contributor guide

Open the contributing guide

Research direction

Read the timeout check in app/scripts/controllers/metametrics-controller.ts alongside the fragment contract in shared/constants/metametrics.ts. Confirm how timeout and lastUpdated units are represented, then verify that the abandonment check compares elapsed time using matching units. Done means the check no longer treats a millisecond timestamp as seconds and preserves the documented timeout behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
analytics
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.